From d68ae710967a037c3f0bad8327845e60fb5c1de2 Mon Sep 17 00:00:00 2001 From: Marco Collovati Date: Thu, 19 Dec 2024 09:27:50 +0100 Subject: [PATCH] fix: trigger refresh on hotswap only for redefined classes (#20684) Prevent hotswapper to trigger a refresh when the classes are loaded for the first time. Refreshing a view should make sense only if a project class has been modified. The only exception is auto layout classes, that must be applied even if they are not directly references in the component tree. Fixes #20680 Fixes #20681 --- .../com/vaadin/flow/hotswap/Hotswapper.java | 79 ++++--- .../vaadin/flow/hotswap/HotswapperTest.java | 213 ++++++++++++++++++ 2 files changed, 257 insertions(+), 35 deletions(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/hotswap/Hotswapper.java b/flow-server/src/main/java/com/vaadin/flow/hotswap/Hotswapper.java index 905592dd7ee..cc40fb440ca 100644 --- a/flow-server/src/main/java/com/vaadin/flow/hotswap/Hotswapper.java +++ b/flow-server/src/main/java/com/vaadin/flow/hotswap/Hotswapper.java @@ -260,7 +260,7 @@ private void onHotswapInternal(HashSet> classes, } } EnumMap> refreshActions = computeRefreshStrategies( - vaadinSessions, classes); + classes, redefined); boolean uiTreeNeedsRefresh = !refreshActions.isEmpty(); if (forceBrowserReload || uiTreeNeedsRefresh) { triggerClientUpdate(refreshActions, forceBrowserReload); @@ -306,13 +306,12 @@ private enum UIRefreshStrategy { } private EnumMap> computeRefreshStrategies( - Set vaadinSessions, Set> changedClasses) { + Set> changedClasses, boolean redefined) { EnumMap> uisToRefresh = new EnumMap<>( UIRefreshStrategy.class); - forEachActiveUI(ui -> uisToRefresh - .computeIfAbsent(computeRefreshStrategy(ui, changedClasses), - k -> new ArrayList<>()) - .add(ui)); + forEachActiveUI(ui -> uisToRefresh.computeIfAbsent( + computeRefreshStrategy(ui, changedClasses, redefined), + k -> new ArrayList<>()).add(ui)); uisToRefresh.remove(UIRefreshStrategy.SKIP); return uisToRefresh; @@ -331,7 +330,7 @@ private void forEachActiveUI(Consumer consumer) { } private UIRefreshStrategy computeRefreshStrategy(UI ui, - Set> changedClasses) { + Set> changedClasses, boolean redefined) { List targetsChain = new ArrayList<>( ui.getActiveRouterTargetsChain()); if (targetsChain.isEmpty()) { @@ -350,21 +349,30 @@ private UIRefreshStrategy computeRefreshStrategy(UI ui, .distinct().toList(); UIRefreshStrategy refreshStrategy; - // A full chain refresh should be triggered if there are modal - // components, since they could be attached to UI or parent layouts - if (ui.hasModalComponent()) { - refreshStrategy = UIRefreshStrategy.PUSH_REFRESH_CHAIN; - } else if (!targetChainChangedItems.isEmpty()) { - refreshStrategy = targetChainChangedItems.stream() - .allMatch(chainItem -> chainItem == route) - ? UIRefreshStrategy.PUSH_REFRESH_ROUTE - : UIRefreshStrategy.PUSH_REFRESH_CHAIN; + if (redefined) { + // A full chain refresh should be triggered if there are modal + // components, since they could be attached to UI or parent layouts + if (ui.hasModalComponent()) { + refreshStrategy = UIRefreshStrategy.PUSH_REFRESH_CHAIN; + } else if (!targetChainChangedItems.isEmpty()) { + refreshStrategy = targetChainChangedItems.stream() + .allMatch(chainItem -> chainItem == route) + ? UIRefreshStrategy.PUSH_REFRESH_ROUTE + : UIRefreshStrategy.PUSH_REFRESH_CHAIN; + } else { + // Look into the UI tree to find if any component is instance of + // a changed class. If so, detect its parent route or layout to + // determine the refresh strategy. + refreshStrategy = computeRefreshStrategyForUITree(ui, + changedClasses, targetsChain, route); + } } else { - // Look into the UI tree to find if any component is instance of - // a changed class. If so, detect its parent route or layout to - // determine the refresh strategy. - refreshStrategy = computeRefreshStrategyForUITree(ui, - changedClasses, targetsChain, route); + // prevent refresh for classes loaded for the first time since + // it shouldn't impact current view, unless they are newly defined + // auto layouts. + // For example, it prevents refresh caused by Dialog related classes + // loaded for the first time when the dialog is opened + refreshStrategy = UIRefreshStrategy.SKIP; } // A different layout might have been applied after hotswap @@ -374,20 +382,21 @@ private UIRefreshStrategy computeRefreshStrategy(UI ui, String currentPath = ui.getActiveViewLocation().getPath(); RouteTarget routeTarget = registry .getNavigationRouteTarget(currentPath).getRouteTarget(); - if (routeTarget != null && ( - // parent layout changed - routeTarget.getParentLayouts().stream() - .anyMatch(changedClasses::contains) || - // applied auto layout changed - RouteUtil.isAutolayoutEnabled(routeTarget.getTarget(), - currentPath) - && registry.hasLayout(currentPath) - && RouteUtil - .collectRouteParentLayouts( - registry.getLayout(currentPath)) - .stream() - .anyMatch(changedClasses::contains))) { - refreshStrategy = UIRefreshStrategy.PUSH_REFRESH_CHAIN; + if (routeTarget != null) { + // parent layout changed + if ((redefined && routeTarget.getParentLayouts().stream() + .anyMatch(changedClasses::contains)) || + // applied auto layout changed or added + RouteUtil.isAutolayoutEnabled(routeTarget.getTarget(), + currentPath) + && registry.hasLayout(currentPath) + && RouteUtil + .collectRouteParentLayouts( + registry.getLayout(currentPath)) + .stream() + .anyMatch(changedClasses::contains)) { + refreshStrategy = UIRefreshStrategy.PUSH_REFRESH_CHAIN; + } } } diff --git a/flow-server/src/test/java/com/vaadin/flow/hotswap/HotswapperTest.java b/flow-server/src/test/java/com/vaadin/flow/hotswap/HotswapperTest.java index b46c9615c4d..70181a1de2e 100644 --- a/flow-server/src/test/java/com/vaadin/flow/hotswap/HotswapperTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/hotswap/HotswapperTest.java @@ -347,6 +347,27 @@ class AutoLayout extends Component implements RouterLayout { Mockito.verify(liveReload).refresh(anyBoolean()); } + @Test + public void onHotswap_pushDisabled_autoLayoutClassFirstLoaded_UINotRefreshedButLiveReloadTriggered() + throws ServiceException { + VaadinSession session = createMockVaadinSession(); + hotswapper.sessionInit(new SessionInitEvent(service, session, null)); + RefreshTestingUI ui = initUIAndNavigateTo(session, + MyAutoLayoutRoute.class); + + @Layout + class AutoLayout extends Component implements RouterLayout { + } + ApplicationRouteRegistry.getInstance(service.getContext()) + .setLayout(AutoLayout.class); + + hotswapper.onHotswap(new String[] { AutoLayout.class.getName() }, + false); + + ui.assertNotRefreshed(); + Mockito.verify(liveReload).refresh(anyBoolean()); + } + @Test public void onHotswap_pushDisabled_parentAutoLayoutClassChanged_UINotRefreshedButLiveReloadTriggered() throws ServiceException { @@ -370,6 +391,30 @@ class AutoLayout extends Component implements RouterLayout { Mockito.verify(liveReload).refresh(anyBoolean()); } + @Test + public void onHotswap_pushDisabled_parentAutoLayoutClassFirstLoaded_UINotRefreshedButLiveReloadTriggered() + throws ServiceException { + + @Layout("level1") + @ParentLayout(MyLayout.class) + class AutoLayout extends Component implements RouterLayout { + } + + VaadinSession session = createMockVaadinSession(); + hotswapper.sessionInit(new SessionInitEvent(service, session, null)); + RefreshTestingUI ui = initUIAndNavigateTo(session, + MyAutoLayoutRoute.class, "level1/view"); + + ApplicationRouteRegistry.getInstance(service.getContext()) + .setLayout(AutoLayout.class); + + hotswapper.onHotswap(new String[] { AutoLayout.class.getName() }, + false); + + ui.assertNotRefreshed(); + Mockito.verify(liveReload).refresh(anyBoolean()); + } + @Test public void onHotswap_pushDisabled_routeTargetChainChanged_UINotRefreshedButLiveReloadTriggered() throws ServiceException { @@ -411,6 +456,22 @@ public void onHotswap_pushDisabled_routeChildClassChanged_UINotRefreshedButLiveR Mockito.verify(liveReload).refresh(anyBoolean()); } + @Test + public void onHotswap_pushDisabled_routeChildClassFirstLoaded_skipLiveReloadAndUIRefresh() + throws ServiceException { + VaadinSession session = createMockVaadinSession(); + hotswapper.sessionInit(new SessionInitEvent(service, session, null)); + RefreshTestingUI ui = initUIAndNavigateTo(session, + MyRouteWithChild.class); + + hotswapper.onHotswap(new String[] { MyComponent.class.getName() }, + false); + + ui.assertNotRefreshed(); + Mockito.verify(liveReload, never()).reload(); + Mockito.verify(liveReload, never()).refresh(anyBoolean()); + } + @Test public void onHotswap_pushDisabled_layoutChildClassChanged_UINotRefreshedButLiveReloadTriggered() throws ServiceException { @@ -426,6 +487,22 @@ public void onHotswap_pushDisabled_layoutChildClassChanged_UINotRefreshedButLive Mockito.verify(liveReload).refresh(anyBoolean()); } + @Test + public void onHotswap_pushDisabled_layoutChildClassFirstLoaded_skipLiveReloadAndUIRefresh() + throws ServiceException { + VaadinSession session = createMockVaadinSession(); + hotswapper.sessionInit(new SessionInitEvent(service, session, null)); + RefreshTestingUI ui = initUIAndNavigateTo(session, MyRoute.class, + MyLayoutWithChild.class); + + hotswapper.onHotswap(new String[] { MyComponent.class.getName() }, + false); + + ui.assertNotRefreshed(); + Mockito.verify(liveReload, never()).reload(); + Mockito.verify(liveReload, never()).refresh(anyBoolean()); + } + @Test public void onHotswap_pushDisabled_routeAndLayoutClassesChanged_UINotRefreshedButLiveReloadTriggered() throws ServiceException { @@ -441,6 +518,22 @@ public void onHotswap_pushDisabled_routeAndLayoutClassesChanged_UINotRefreshedBu Mockito.verify(liveReload).refresh(anyBoolean()); } + @Test + public void onHotswap_pushDisabled_routeAndLayoutClassesFirstLoaded_skipLiveReloadAndUIRefresh() + throws ServiceException { + VaadinSession session = createMockVaadinSession(); + hotswapper.sessionInit(new SessionInitEvent(service, session, null)); + RefreshTestingUI ui = initUIAndNavigateTo(session, MyRoute.class, + MyLayout.class); + + hotswapper.onHotswap(new String[] { MyRoute.class.getName(), + MyLayout.class.getName() }, false); + + ui.assertNotRefreshed(); + Mockito.verify(liveReload, never()).reload(); + Mockito.verify(liveReload, never()).refresh(anyBoolean()); + } + @Test public void onHotswap_pushDisabled_routeAndLayoutChildClassChanged_UINotRefreshedButLiveReloadTriggered() throws ServiceException { @@ -456,6 +549,23 @@ public void onHotswap_pushDisabled_routeAndLayoutChildClassChanged_UINotRefreshe Mockito.verify(liveReload).refresh(anyBoolean()); } + @Test + public void onHotswap_pushDisabled_routeAndLayoutChildClassFirstLoaded_skipLiveReloadAndUIRefresh() + throws ServiceException { + VaadinSession session = createMockVaadinSession(); + hotswapper.sessionInit(new SessionInitEvent(service, session, null)); + RefreshTestingUI ui = initUIAndNavigateTo(session, + MyRouteWithChild.class, MyLayoutWithChild.class); + + hotswapper.onHotswap(new String[] { MyComponent.class.getName() }, + false); + + ui.assertNotRefreshed(); + Mockito.verify(liveReload, never()).reload(); + Mockito.verify(liveReload, never()).refresh(anyBoolean()); + + } + @Test public void onHotswap_pushDisabled_changedClassNotInUITree_skipLiveReloadAndUIRefresh() throws ServiceException { @@ -544,6 +654,29 @@ class AutoLayout extends Component implements RouterLayout { Mockito.verify(liveReload, never()).refresh(anyBoolean()); } + @Test + public void onHotswap_pushEnabled_autoLayoutClassFirstLoaded_activeChainRefreshed() + throws ServiceException { + VaadinSession session = createMockVaadinSession(); + hotswapper.sessionInit(new SessionInitEvent(service, session, null)); + RefreshTestingUI ui = initUIAndNavigateTo(session, + MyAutoLayoutRoute.class); + ui.enablePush(); + + @Layout + class AutoLayout extends Component implements RouterLayout { + } + ApplicationRouteRegistry.getInstance(service.getContext()) + .setLayout(AutoLayout.class); + + hotswapper.onHotswap(new String[] { AutoLayout.class.getName() }, + false); + + ui.assertChainRefreshed(); + Mockito.verify(liveReload, never()).reload(); + Mockito.verify(liveReload, never()).refresh(anyBoolean()); + } + @Test public void onHotswap_pushEnabled_parentAutoLayoutClassChanged_activeChainRefreshed() throws ServiceException { @@ -569,6 +702,32 @@ class AutoLayout extends Component implements RouterLayout { Mockito.verify(liveReload, never()).refresh(anyBoolean()); } + @Test + public void onHotswap_pushEnabled_parentAutoLayoutClassFirstLoaded_activeChainRefreshed() + throws ServiceException { + + @Layout("level1") + @ParentLayout(MyLayout.class) + class AutoLayout extends Component implements RouterLayout { + } + + VaadinSession session = createMockVaadinSession(); + hotswapper.sessionInit(new SessionInitEvent(service, session, null)); + RefreshTestingUI ui = initUIAndNavigateTo(session, + MyAutoLayoutRoute.class, "level1/view"); + ui.enablePush(); + + ApplicationRouteRegistry.getInstance(service.getContext()) + .setLayout(AutoLayout.class); + + hotswapper.onHotswap(new String[] { AutoLayout.class.getName() }, + false); + + ui.assertChainRefreshed(); + Mockito.verify(liveReload, never()).reload(); + Mockito.verify(liveReload, never()).refresh(anyBoolean()); + } + @Test public void onHotswap_pushEnabled_routeTargetChainChanged_activeChainRefreshed() throws ServiceException { @@ -632,6 +791,24 @@ public void onHotswap_pushEnabled_routeChildrenClassChanged_routeRefreshed() Mockito.verify(liveReload, never()).refresh(anyBoolean()); } + @Test + public void onHotswap_pushEnabled_routeChildrenClassFirstLoaded_skipLiveReloadAndUIRefresh() + throws ServiceException { + VaadinSession session = createMockVaadinSession(); + hotswapper.sessionInit(new SessionInitEvent(service, session, null)); + + RefreshTestingUI ui = initUIAndNavigateTo(session, + MyRouteWithChild.class); + ui.enablePush(); + + hotswapper.onHotswap(new String[] { MyComponent.class.getName() }, + false); + + ui.assertNotRefreshed(); + Mockito.verify(liveReload, never()).reload(); + Mockito.verify(liveReload, never()).refresh(anyBoolean()); + } + @Test public void onHotswap_pushEnabled_layoutChildrenClassChanged_activeChainRefreshed() throws ServiceException { @@ -650,6 +827,24 @@ public void onHotswap_pushEnabled_layoutChildrenClassChanged_activeChainRefreshe Mockito.verify(liveReload, never()).refresh(anyBoolean()); } + @Test + public void onHotswap_pushEnabled_layoutChildrenClassFirstLoaded_skipLiveReloadAndUIRefresh() + throws ServiceException { + VaadinSession session = createMockVaadinSession(); + hotswapper.sessionInit(new SessionInitEvent(service, session, null)); + + RefreshTestingUI ui = initUIAndNavigateTo(session, MyRoute.class, + MyLayoutWithChild.class); + ui.enablePush(); + + hotswapper.onHotswap(new String[] { MyComponent.class.getName() }, + false); + + ui.assertNotRefreshed(); + Mockito.verify(liveReload, never()).reload(); + Mockito.verify(liveReload, never()).refresh(anyBoolean()); + } + @Test public void onHotswap_pushEnabled_routeAndLayoutClassesChanged_activeChainRefreshed() throws ServiceException { @@ -686,6 +881,24 @@ public void onHotswap_pushEnabled_routeAndLayoutChildClassChanged_activeChainRef Mockito.verify(liveReload, never()).refresh(anyBoolean()); } + @Test + public void onHotswap_pushEnabled_routeAndLayoutChildClassFirstLoaded_skipLiveReloadAndUIRefresh() + throws ServiceException { + VaadinSession session = createMockVaadinSession(); + hotswapper.sessionInit(new SessionInitEvent(service, session, null)); + + RefreshTestingUI ui = initUIAndNavigateTo(session, + MyRouteWithChild.class, MyLayoutWithChild.class); + ui.enablePush(); + + hotswapper.onHotswap(new String[] { MyComponent.class.getName() }, + false); + + ui.assertNotRefreshed(); + Mockito.verify(liveReload, never()).reload(); + Mockito.verify(liveReload, never()).refresh(anyBoolean()); + } + @Test public void onHotswap_pushEnabled_changedClassNotInUITree_skipRefresh() throws ServiceException {