Execute BeforeEnter listeners starting with top parent. #7130

bogdanudrescu · 2019-12-11T09:44:37Z

Route layouts and target component are created right before the event is fired. So if a parent layout beforeEnter redirects, then the rest of the layouts in the chain and the component target itself won't be created.

Before this change the order of events was as follow:

target component and layouts are created.
setParameter event is triggered.
Global handlers are invoked.
BeforeEnter events is triggered starting with target component and ending with the first layout.

After the change:

Global handlers are invoked.
components are created starting with first layout in the chain and ending with the target component.
each BeforeEvent is triggered right after the layout or component is created (if implementing BeforeEventHandler) and before the next layout or component in the chain is created.
setParameter is invoked right before the target's beforeEnter or any of its children or last if no BeforeEnterHandler is implemented in target or its children.

This PR also fixes the case when a route layout redirects to the navigation target currently navigating to.

This change is

caalador

Reviewable status: 4 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @bogdanudrescu, @caalador, and @Legioth)

flow-server/src/main/java/com/vaadin/flow/router/BeforeEvent.java, line 439 at r8 (raw file):

     * @deprecated use {@link #getRerouteTargetType()} instead.
     */
    @deprecated

It should be @Deprecated

flow-server/src/main/java/com/vaadin/flow/router/internal/AbstractNavigationStateRenderer.java, line 466 at r7 (raw file):

Previously, bogdanudrescu (Bogdan Udrescu) wrote…

Then may we leave it as it is? Inside createChain... we need to reverse it back and forth since there it's either created or it comes in with target first and we need to send the events the other way then we need to output it with target first, although it's create starting with root.

Could we just part them into two methods where one handles new chain creation and one handles a prepopulated one?

bogdanudrescu

Reviewable status: 4 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @bogdanudrescu, @caalador, and @Legioth)

flow-server/src/main/java/com/vaadin/flow/router/internal/AbstractNavigationStateRenderer.java, line 466 at r7 (raw file):

Previously, caalador wrote…

Could we just part them into two methods where one handles new chain creation and one handles a prepopulated one?

One moment, I'll make a bit of changes and add one more test.

bogdanudrescu

Reviewable status: 4 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @bogdanudrescu, @caalador, and @Legioth)

flow-server/src/main/java/com/vaadin/flow/router/internal/AbstractNavigationStateRenderer.java, line 466 at r7 (raw file):

Previously, bogdanudrescu (Bogdan Udrescu) wrote…

One moment, I'll make a bit of changes and add one more test.

Yes, we can do that too.

bogdanudrescu

Reviewable status: 4 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @bogdanudrescu, @caalador, and @Legioth)

flow-server/src/main/java/com/vaadin/flow/router/BeforeEvent.java, line 439 at r8 (raw file):

Previously, caalador wrote…

It should be @Deprecated

Done.

caalador

Please ask someone else from the to also have a look.

Reviewed 3 of 3 files at r9.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @bogdanudrescu and @Legioth)

joheriks

Reviewable status: 7 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @bogdanudrescu and @Legioth)

flow-server/src/main/java/com/vaadin/flow/router/BeforeEvent.java, line 445 at r9 (raw file):

    /**
     * Get the route target for rerouting.

With his API addition, there will be one method called getRerouteTarget() and one calledgetRerouteTargetType(), and the JavaDoc for getRerouteTarget() also says effectively ´gets the reroute target`. It could be good to write that this methods gets route target type for better consistency in the terminology.

flow-server/src/main/java/com/vaadin/flow/router/EventUtil.java, line 132 at r9 (raw file):

                .flatMap(chainRoot -> collectBeforeEnterObserversStream(
                        chainRoot.getElement(),
                        getElements(childrenExclusions)))

Evaluating getElements(childrenExclusions) inside the lambda is suboptimal as it leads to the childrenExclusionsstream being materialized multiple times. The subexpression could be lifted outside the flatMap expression.

flow-server/src/main/java/com/vaadin/flow/router/internal/AbstractNavigationStateRenderer.java, line 82 at r9 (raw file):

    private Postpone postponed = null;

    private LocationChangeEvent locationChangeEvent;

Not blocking, but maybe initialize this also explicitly to null for clarity.

flow-server/src/main/java/com/vaadin/flow/router/internal/AbstractNavigationStateRenderer.java, line 196 at r9 (raw file):

                // We're returning because the preserved chain is not ready to
                // be used as is, and requires client data requested within
                // `createOrRehandlePreserveOnRefreshComponent`. Once the data

Since method createOrRehandlerPreserveOnRefreshComponent is renamed in the change, update also the comment to reduce confusion.

flow-server/src/main/java/com/vaadin/flow/router/internal/AbstractNavigationStateRenderer.java, line 593 at r9 (raw file):

    /*
     * Check whether the eventHandler is the component itself ot a child of it.

Typo: "ot" -> "or"

flow-server/src/main/java/com/vaadin/flow/router/internal/AbstractNavigationStateRenderer.java, line 595 at r9 (raw file):

     * Check whether the eventHandler is the component itself ot a child of it.
     */
    private boolean isEqualsOrChild(BeforeEnterHandler eventHandler,

Method could be static and the name improved, since the method doesn't return true if the arguments are equal (but rather, if their contained elements are equal).

bogdanudrescu

Reviewable status: 7 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @bogdanudrescu, @caalador, @joheriks, and @Legioth)

flow-server/src/main/java/com/vaadin/flow/router/BeforeEvent.java, line 445 at r9 (raw file):