Repository navigation
refactor: send hotswap UI refresh through a JS definition - #26979
mcollovati wants to merge 1 commit into
Conversation
Type of change
How to testNo integration test view covers the push hotswap path. In a dev mode app with
Test coverage
|
vaadin-review-bot
left a comment
vaadin-review-bot
left a comment
There was a problem hiding this comment.
Reviewed the changes — left 2 comments.
| Finding | |
|---|---|
Spring apps that set vaadin.allowed-packages never scan RefreshUiJs, so the hotswap refresh breaks for them. |
|
| 🧪 | No test checks the event name or payload sent to the browser anymore. |
| * For internal use only. May be renamed or removed in a future release. | ||
| */ | ||
| @JsDefinition | ||
| public interface RefreshUiJs extends Serializable { |
There was a problem hiding this comment.
vaadin.allowed-packages never scan RefreshUiJs, so the hotswap refresh breaks for them.
When allowed packages are set, Spring dev mode scans only the app packages, the custom list and DEFAULT_SCAN_ONLY in VaadinServletContextInitializer. That list was widened on purpose to cover the framework's JS definitions (see its comment), but it has no entry for com.vaadin.base.devserver. For these apps:
- The generated definitions file leaves out
RefreshUiJs, so the push refresh after a hotswap has no function to run in the browser. Before this PR it worked, because it was sent as a string. - The generated content no longer matches the hash of the prebuilt dev bundle, which includes the definition. That forces a rebundle, which goes against the PR's claim that dev mode apps do not rebundle.
To fix it, add the package (or Hotswapper's package) to DEFAULT_SCAN_ONLY. The guard test onStartup_allowedPackagesSet_devModeScansEveryFrameworkJsDefinition only looks for definitions under com.vaadin.flow, which is why it misses this one. Widen it as well.
Hotswapper.java:824 · correctness
| }).when(pageSpy).executeJs(anyString(), any(Object[].class)); | ||
| // Intercept the request to the browser to check whether a UI | ||
| // refresh has been asked for, and for which part of the chain. | ||
| Hotswapper.RefreshUiJs refreshUiJs = fullRefresh -> refreshRouteChainRequested = fullRefresh; |
There was a problem hiding this comment.
🧪 No test checks the event name or payload sent to the browser anymore.
The old test matched the JavaScript text against vaadin-refresh-ui and the detail.fullRefresh shape. The new stub replaces the whole definition with a lambda. A typo in the @JsExpression event name or detail key would now pass every test, and the dev tools would just stop refreshing. A small check on the expression, or a test against the generated snippet, would bring that coverage back.
HotswapperTest.java:1680 · tests
|
@mcollovati this "chore:"/"test:"/"refactor:" PR has been approved automatically, but auto merge has not been enabled for it. You can enable the auto merge by yourself, or ask another member for another review. [Message is sent from bot] |
|
refactor · vaadin-dev-server · dev mode apps with push that hotswap a class
Background — JS definitions. An interface annotated with
@JsDefinitiondeclares JavaScript that the build puts into the frontend bundle, andexecuteJs(Class)calls it by a hash instead of sending JavaScript text. The client then never has to build a function from a string, so the call works under a content security policy withoutunsafe-eval.After a hotswap, the request that tells the browser to refresh a UI with push was the last JavaScript string sent with
executeJsin Flow. It now uses a JS definition, like every other call, so it no longer needs eval in the browser.Risks:
Hotswapper.RefreshUiJs, public because every JS definition is, and documented as internal.Context. The definition lives in
vaadin-dev-server, notflow-server, so production bundles do not carry it. The prebuilt dev bundle in the platform repo depends onvaadin-dev-server, so it includes the definition and dev mode apps do not rebundle.Hotswapper.RefreshUiJs, which fires thevaadin-refresh-uiwindow event withfullRefreshas a parameterString.format+executeJs(String)call inHotswapperwithexecuteJs(RefreshUiJs.class).refresh(fullRefresh)HotswapperTest.RefreshTestingUIto record the flag fromRefreshUiJsinstead of matching the JavaScript text