Add support for LiveReload without browser extensions #43697
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
| */ | ||
| @Configuration(proxyBeanMethods = false) | ||
| @ConditionalOnWebApplication(type = Type.SERVLET) | ||
| class LiveReloadServletConfiguration { |
There was a problem hiding this comment.
This configuration contains Servlet-specific bits needed to set up everything. If something like this is a viable approach, I assume WebFlux-specific equivalent is needed as well (I've never used Dev Tools with WebFlux so I'm unaware if there are any limitations).
| private final String scriptSnippet; | ||
|
|
||
| public LiveReloadScriptFilter(int liveReloadPort) { | ||
| this.scriptSnippet = String.format("<script src=\"/livereload.js?port=%d\"></script>", liveReloadPort); |
There was a problem hiding this comment.
Perhaps something to consider is embedding the script in what LiveReload.js readme refers to as slightly smarter way, see https://github.com/livereload/livereload-js?tab=readme-ov-file#using-livereloadjs.
There was a problem hiding this comment.
This updates the existing livereload.js to latest version from https://github.com/livereload/livereload-js.
| @@ -284,8 +284,6 @@ If you find such a problem, you need to request a fix with the original authors. | |||
| == LiveReload | |||
|
|
|||
| The `spring-boot-devtools` module includes an embedded LiveReload server that can be used to trigger a browser refresh when a resource is changed. | |||
| LiveReload browser extensions are freely available for Chrome, Firefox and Safari. | |||
There was a problem hiding this comment.
Not sure whether it's needed to explicitly call out here that browser extensions are no longer needed and Spring Boot provides everything needed.
|
Unfortunately I think the filter might generate invalid HTML. If I'm reading the code correctly then it inserts the I guess the filter could parse the HTML and try to insert the script in the correct location. The problem there is that we might need to parse all responses just to tell if we're getting HTML. On top of that, the WebFlux side is also going to be pretty hard to address. Flagging for team attention in case anyone else has any bright ideas. |
philwebb
added
for: team-attention
That's true. I didn't (yet) put much effort into making the filter implementation robust as I wanted to pick up some feedback on the high-level approach first. Looking into what some other web frameworks do this shouldn't be too hard - for example Hugo injects the
The filter already takes response content type into consideration (requiring it to be |
I think it can't determine the content type until it's called the filter chain. That means we probably need to capture content for all requests in case they're HTML and need the script tag injected. |
|
Yes, but that (wrapping) is different than parsing every response - that can still only be done if the response indeed is I've experimented with my PoC project a bit and got something (again very simple, not robust) working using I've also pushed those changes here. |
This commit improves Dev Tools live reload capabilities by adding support for appending LiveReload.js script to rendered web pages. See spring-projectsgh-32111 Signed-off-by: Vedran Pavic <[email protected]>
|
The filter should now work with any valid HTML and inserts LiveReload script element into head as the first child element even if the |
|
@philwebb could you share the reason this is marked as on-hold? The concerns you expressed in #43697 (comment) are addressed, with the exception of WebFlux support. I haven't looked at that part yet but if that's the only thing blocking this from moving forward I'll try to find some time to do it. |
|
Sorry for the delay in replying @vpavic, I've been at Devnexus and then focusing on another piece of work. We have quite serious reservations about attempting to automatically insert the live reload scripts into applications using a filter. We think that cost of intercepting and buffering every request, along with the risk of rewriting HTML on the fly is ultimately too high. We're now thinking that if we are to offer live-reload support, we should perhaps offer it as helpers that users can insert into their templates directly. Whilst this won't be quite as seamless an experience, we think it might be safer and more aligned with the way that other frameworks handle live reload. I'm afraid we're going to decline this PR and leave #32111 open for a little while longer. This is a problem that we want to solve, but currently our focus needs to be on the Spring Boot 4.0 work. I'm sorry for any wasted effort, and I hope we can eventually find a solution that makes everybody happy. |
mhalbritter
added
status: declined
|
The approach taken in this PR is I believe equivalent to one Hugo and Jekyll take - two of the most popular static site generators. If such approach is not problematic for them, could you maybe clarify a bit more what are these quite serious reservations?
The implementation proposed here leverages Semi-related remark regarding Spring Boot's Livereload.js support: some static site generators, for example Jekyll, don't enable Livereload by default when development server is started - rather they require developers to explicitly enable it (using |
Our main concern is that we're changing the developed application behind the scenes, potentially without the user really being aware that it's going on. The filter needs to inject itself into the HTML, which means we either need to parse the HTML or use a regex solution as you've suggested in the PR. Either way, it feels a little risky and we think that having the user opt-in with the helper methods would be safer.
It is a concern for At least the
Yeah, that's a nice idea. The filter would be a lot less concerning if users opt-in. I see it then being something similar to the helper utilities. We can also put a big warning on the property to warn folks not to use it if they're sending large amounts of data. Another thought I had was a more intelligent version of |
|
Thanks for sharing additional details.
I agree with this, but in many ways this is already the case with DevTools - I've actually expressed similar concerns earlier in #29406. I remember that Framework issue you linked, though it's a bit extreme example - dynamically generated responses (JSON, HTML) will very rarely be large enough for this to be an issue in practice. And cache busting mechanism IMO targets responses that are of completely different nature (static). Would you reconsider this PR for 4.0 if a separate change flips the default value of |
Yes, they probably won't be, but the problem is we need to inspect/cache every request including those that aren't JSON/HTML. The threshold idea would certainly help with that.
I think if we could flip default and have the threshold change we'd be much more likely to accept the change. I can't make any promises for 4.0 just due to the amount of other work we need to get into that release. |
|
I would also argue that 4k lines of unmaintained javascript is a liability for our project. I'm sure other projects think it's fine but given the feature scope we want here, a simpler version would be fine: reloading the entire page when we receive a reload event over WebSockets. |
Similar to what
In #32111 (comment) I propsed to pull it in as a WebJar dependency (potentially shaded) but in the subsequent comment it was suggested to just include it in the project as the current version is. |
This commit improves Dev Tools live reload capabilities by adding support for appending LiveReload.js script to rendered web pages.
See gh-32111