feat: retain local signal values during hotswap#23854
Conversation
Automatically transfer local signal values from old view fields to matching fields in the new view during hotswap refresh. Fixes: #23830
flow-server/src/main/java/com/vaadin/flow/router/internal/AbstractNavigationStateRenderer.java
Show resolved
Hide resolved
Legioth
left a comment
There was a problem hiding this comment.
Will also look at the tests but posting findings from reviewing the implementation right away
| } | ||
|
|
||
| private static boolean isLocalSignalType(Class<?> type) { | ||
| return ValueSignal.class.isAssignableFrom(type) |
There was a problem hiding this comment.
Maybe check for AbstractLocalSignal instead so that any future subclasses are also automatically covered?
|
|
||
| private static void transferField(HasElement oldInstance, | ||
| HasElement newInstance, Field newField) throws Exception { | ||
| Field oldField = findField(oldInstance.getClass(), newField.getName()); |
There was a problem hiding this comment.
Looking through the entire type hierarchy might lead to problems in the edge case case when multiple classes in the hierarchy have a private field with the same name. Would it work to also pass in newClass from the transferLocalSignalValues method and somehow use that either for the lookup or for additional verification?
There was a problem hiding this comment.
It's a bit more complicated even, because we can't mix old and new class to read fields since class may have changed. Need to iterate both class hierarchies in parallel instead.
There was a problem hiding this comment.
What about matching fields only if the fully qualified class names match?
There was a problem hiding this comment.
That's another option. Both probably works, but this might be more efficient even.
There was a problem hiding this comment.
Efficiency is relatively irrelevant considering all the other processing that happens in a hotswap scenario. I'm mostly thinking of correctness to avoid accidentally mixing up signal values.
| if (oldField == null) { | ||
| return; | ||
| } | ||
| if (!isLocalSignalType(oldField.getType())) { |
There was a problem hiding this comment.
Could check that it's the same type and not just any local signal type. That would also allow slightly simplifying things further down.
| ? Optional.empty() | ||
| : findActiveRouteTarget(event, isRouteTargetType); | ||
| return (T) currentInstance.orElseGet( | ||
| T result = (T) currentInstance.orElseGet( |
There was a problem hiding this comment.
Could use a more descriptive variable name, e.g. routeTarget
| var newView = new ViewWithListSignal(); | ||
| SignalFieldTransfer.transferLocalSignalValues(oldView, newView); | ||
|
|
||
| var values = newView.items.peek().stream().map(ValueSignal::peek) |
There was a problem hiding this comment.
Can be simplified to newView.items.peekValues().toList()
| } | ||
|
|
||
| @Test | ||
| void exceptionFromSignalSetIsLogged() { |
There was a problem hiding this comment.
Misleading method name since it doesn't assert anything about logging
vaadin-dev-server/src/test/java/com/vaadin/base/devserver/hotswap/HotswapSignalsTest.java
Show resolved
Hide resolved
Legioth
left a comment
There was a problem hiding this comment.
One minimal stylistic remark. Feel free to merge without adjusting or to adjust and then merge right away without waiting for a follow-up review.
| Class<?> current = targetClazz; | ||
| while (current != null && current != Object.class) { | ||
| try { | ||
| if (Objects.equals(current.getName(), |
There was a problem hiding this comment.
I'm confused by the use of Objects.equals here since it implies that either value might be null but I don't see how that could ever happen. (And if still can happen, then I don't think we should match in that case)
There was a problem hiding this comment.
removed unnecessary usage of Object.equals.
|
* feat: retain local signal values during hotswap Automatically transfer local signal values from old view fields to matching fields in the new view during hotswap refresh. Fixes: #23830 * make SignalFieldTransfer Serializable * update javadoc * Add more tests * Add unit test and remove unused constructor * Fix code review findings * remove unnecessary java.util.Objects usage --------- Co-authored-by: Leif Åstrand <leif@vaadin.com>
* feat: retain local signal values during hotswap Automatically transfer local signal values from old view fields to matching fields in the new view during hotswap refresh. Fixes: #23830 * make SignalFieldTransfer Serializable * update javadoc * Add more tests * Add unit test and remove unused constructor * Fix code review findings * remove unnecessary java.util.Objects usage --------- Co-authored-by: Tomi Virtanen <tltv@vaadin.com> Co-authored-by: Leif Åstrand <leif@vaadin.com>



Automatically transfer local signal values from old view fields to matching fields in the new view during hotswap refresh.
Fixes: #23830
This pull request introduces a new mechanism for transferring local signal field values between view instances during a hot-reload (hotswap refresh) in development mode. The main addition is the
SignalFieldTransferutility, which ensures that the state held inValueSignalandListSignalfields is preserved when a view is refreshed, improving the developer experience.The most important changes are:
Signal transfer utility
SignalFieldTransferinflow-server/src/main/java/com/vaadin/flow/internal/SignalFieldTransfer.javathat provides static methods to transfer local signal field values (of typeValueSignalandListSignal) from an old view instance to a new one, matching fields by name and type. This is used to preserve signal state during hot-reload in development.Integration with navigation
AbstractNavigationStateRenderer#getRouteTargetto callSignalFieldTransfer.transferLocalSignalValueswhen a view is refreshed (triggered byNavigationTrigger.REFRESH_ROUTE) and the application is not in production mode, ensuring signal state is transferred during development hot-reloads.SignalFieldTransferclass inAbstractNavigationStateRenderer.javato enable the above integration.