-
Notifications
You must be signed in to change notification settings - Fork 191
fix: Use file watcher and inline stylesheets for hot reload #22851
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…older - Uses file watcher for stylesheet changes to replace hotswapper File watcher listens for css files change in META-INF/resources/, remove this feature from hot-swapper - StyleSheet bundler implementation. Implemented server-side bundling utility for @Stylesheet URLs locating CSS in common source roots and bundling with CssBundler.inlineImports. Added PublicStyleSheetBundler class with methods to create, bundle, and normalize URLs. - Add active stylesheets tracker. Implemented ActiveStyleSheetTracker to track active stylesheet URLs per session and globally. Wired tracker updates in StyleSheetHotswapper for AppShell URLs and per-UI stylesheet changes. - Bundle CSS when watching Implements live bundling and push of public CSS changes in dev-bundle mode using PublicResourcesLiveUpdater and ActiveStyleSheetTracker. It ensures CSS edits trigger server-side re-bundling and live updates to the browser. - Create/update style tags Enhances the client-side live-reload handler to apply inline bundled CSS immediately. Updated the onUpdate function to create or update style tags and disable duplicate link tags. - Initial sync for newly added/removed @Stylesheet Implements immediate server push of inlined bundled CSS for added @Stylesheet URLs and signaling removal with empty content. The client updates or removes corresponding style tags accordingly.
|
Tested hot-reload manually for:
|
Test Results1 303 files + 3 1 303 suites +3 1h 15m 42s ⏱️ +45s Results for commit 9425e0a. ± Comparison against base commit 1af987a. This pull request removes 2 and adds 17 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
| return; | ||
| } | ||
| if (!file.getName().endsWith(".css")) { | ||
| liveReload.reload(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can cause page unwanted reloads if the IDE uses temp files when saving changes to a file (e.g. styles.css~). Furthermore, the after page reload it might happen that you see the old version of the CSS because of servlet container resource cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added the ignore for files with tilde.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reload happens also if you, for example, drop a new file in the resource folder. This is a bit annoying, but as discussed, it can be an acceptable behavior since adding resources might not be so frequent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reloading doesn't guarantee that the resource is immediately visible as IDE needs time to copy it to output dir. I have no strong opinion, we can remove it right away or remove later on request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove it now. IMO it could be more annoying to have unexpected reloads rather than having to reload manually in few cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
| File entry = new File(root, normalized); | ||
| if (entry.exists() && entry.isFile()) { | ||
| try { | ||
| String bundled = CssBundler.inlineImportsForPublicResources( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there's a bug in resolving URL in nested imports.
Given the following structure and a background-image: url(../../images/viking.png); rule in src/main/resources/META-INF/resources/css/view/nested/nested-imported.css, after the watcher updates the CSS, it results in background-image: url(../images/viking.png); and the images is not shown anymore.
The view is annotated with @StyleSheet("context://css/view/view.css").
view.css imports imported.css that in turn has an import for nested/nested-imported.css
src/main/resources
├── application.properties
├── hotswap-agent.properties
└── META-INF
└── resources
├── css
│ ├── images
│ │ └── viking.png
│ └── view
│ ├── imported.css
│ ├── nested
│ │ └── nested-imported.css
│ └── view.css
└── styles.css
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issues seems to be in the url resolution when a custom servlet mapping is set.
The url is rewritten by CssBundler as url('css/images/viking.png'), but then the request url becomes http://localhost:8888/context/view/css/images/viking.png which server can't server with 404.
Without the servlet mapping this does work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the problem was in the CssBundler that didn't take the context-path into account for resolved urls.
Pushed the fix for it and also tested with Spring-based app with context-path and url-mapping. Works better now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with the latest changes and the images are referenced correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems there is still something broken with URL resolution.
If the application has a context path /app and I change url(css/images/viking.png) to url(http://localhost:8080/app/css/images/viking.png) the result is url('/app/http:/localhost:8080/app/css/images/viking.png')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because of an incorrect regex in CssBundler; see https://github.com/vaadin/flow/pull/22851/files#r2584495007
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pattern has been changed, so should work now.
…le-watcher-and-inline
52d6b54 to
94cb624
Compare
| ui.addAfterNavigationListener(navigationEvent -> { | ||
| UI newUi = navigationEvent.getLocationChangeEvent().getUI(); | ||
| Set<String> allUrls = new LinkedHashSet<>(); | ||
| lookupUrlsForComponents(newUi, allUrls, vaadinService); | ||
| allUrls.forEach(tracker::trackAddForComponent); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could probably be replaced by installing a DependencyFilter on VaadinService initialization event.
DependencyFilters are invoked at every UIDL request, so it should be possible to update the tracker also when a component is added to the page, not only after a navigation to a view.
But this could also be done in a separate PR, as an additional improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do it in a new PR as changes to this PR are already quite big and hard to review.
mcollovati
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A first round of review. I'm now going to test the runtime effects of the changes
flow-server/src/main/java/com/vaadin/flow/component/internal/StyleSheetHotswapper.java
Outdated
Show resolved
Hide resolved
flow-server/src/main/java/com/vaadin/flow/server/frontend/CssBundler.java
Outdated
Show resolved
Hide resolved
flow-server/src/main/java/com/vaadin/flow/server/frontend/CssBundler.java
Outdated
Show resolved
Hide resolved
flow-server/src/main/java/com/vaadin/flow/server/frontend/CssBundler.java
Outdated
Show resolved
Hide resolved
flow-server/src/main/java/com/vaadin/flow/server/frontend/CssBundler.java
Outdated
Show resolved
Hide resolved
flow-server/src/main/java/com/vaadin/flow/server/AppShellRegistry.java
Outdated
Show resolved
Hide resolved
flow-server/src/test/java/com/vaadin/flow/component/internal/StyleSheetHotswapperTest.java
Outdated
Show resolved
Hide resolved
flow-server/src/test/java/com/vaadin/flow/internal/ActiveStyleSheetTrackerTest.java
Show resolved
Hide resolved
flow-server/src/main/java/com/vaadin/flow/server/frontend/CssBundler.java
Show resolved
Hide resolved
| + MAYBE_LAYER_OR_MEDIA_QUERY + WHITE_SPACE + ";"); | ||
|
|
||
| private static Pattern urlPattern = Pattern.compile(URL); | ||
| private static final Pattern protocolPatternForUrls = Pattern |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use uppercase for constants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are also other statics that can be turned to uppercase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
184cbe7 to
18cecd6
Compare
vaadin-dev-server/src/test/java/com/vaadin/base/devserver/PublicStyleSheetBundlerTest.java
Outdated
Show resolved
Hide resolved
mshabarov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed formatting
vaadin-dev-server/src/test/java/com/vaadin/base/devserver/PublicStyleSheetBundlerTest.java
Outdated
Show resolved
Hide resolved
|
|
This ticket/PR has been released with Vaadin 25.0.0-beta9. |



Reworks hot-reload for CSS files referenced from
AtStyleSheetin a similar way as for theme folder hot-reload.Uses file watcher for stylesheet changes to replace hotswapper
File watcher listens for css files change in META-INF/resources/, removes this feature from hot-swapper
StyleSheet bundler implementation
Implemented server-side bundling utility for @Stylesheet URLs locating CSS in common source roots and bundling with CssBundler.inlineImports. Added PublicStyleSheetBundler class with methods to create, bundle, and normalize URLs.
Add active stylesheets tracker
Implements ActiveStyleSheetTracker to track active stylesheet URLs per AppShell and components. The hotswapper populates initial stylesheets to this tracker and also keeps active stylesheets in sync after hot reload. Tracker is used by file watcher to know what style/link tags to update.
Bundle CSS when watching
Implements live bundling and push of public CSS changes in dev-bundle mode using PublicResourcesLiveUpdater and ActiveStyleSheetTracker. It ensures CSS edits trigger server-side re-bundling and live updates to the browser.
Create/update style tags
Enhances the client-side live-reload handler to apply inline bundled CSS immediately. Updated the onUpdate function to create or update style tags and remove duplicate link tags.
Fixes #22837