CCDM: support server-side routing in client bootstrapping #6511

manolo · 2019-09-20T08:23:41Z

This change is

qtdzz

Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @manolo and @qtdzz)

flow-client/src/main/resources/META-INF/resources/frontend/Flow.ts, line 65 at r1 (raw file):

  async start(): Promise<any> {
    await this.flowInit(true);
    document.body.appendChild(this.container);

I had a discussion with @vlukashov, we should put the content of server-side in the body instead of the container. So, the result will be closer to application bootstrapped by server-side.

flow-client/src/main/resources/META-INF/resources/frontend/Flow.ts, line 66 at r1 (raw file):

    await this.flowInit(true);
    document.body.appendChild(this.container);
    return this.flowNavigate({

This also means the first start will take two round trips, right? I wonder if we could somehow modify the init request to include information for the first route so that we only need one round trip for initializing the app and loading the first view.

flow-server/src/main/java/com/vaadin/flow/server/communication/JavaScriptBootstrapHandler.java, line 140 at r1 (raw file):

    @Override
    protected void initializeUIWithRouter(VaadinRequest request, UI ui) {

I have a concern here. If we don't override this method, it will navigate to the current location (i.e. localhost:8080/VAADIN/?v-r=init) after creating the UI.

platosha

Reviewed 1 of 6 files at r1.
Reviewable status: 4 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @manolo)

flow-client/src/main/resources/META-INF/resources/frontend/Flow.ts, line 63 at r1 (raw file):

   * Initialize flow in full page mode and with server side routing.
   */
  async start(): Promise<any> {

any does not give much sense.

Do we have a use case for this method’s return value? If not, should it be declared as Promise<void> perhaps?

Maybe even omit return here:

async start() {
  await this.flowInit();
  document.body.appendChild(this.container);
  this.flowNavigate(/*...*/);
}

TypeScript will infer Promise<void> then.

platosha

Reviewed 1 of 6 files at r1.
Reviewable status: 4 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @manolo)

qtdzz

Reviewed 6 of 6 files at r1.
Reviewable status: 4 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @manolo)

flow-server/src/main/java/com/vaadin/flow/server/communication/JavaScriptBootstrapHandler.java, line 140 at r1 (raw file):

Previously, qtdzz (Tien Nguyen) wrote…

I have a concern here. If we don't override this method, it will navigate to the current location (i.e. localhost:8080/VAADIN/?v-r=init) after creating the UI.

I investigated the failed test ClientIndexHandlerIT.should_preserveView_When_reloadPreservedOnRefreshView. It turns out that this change causes the error. What happened is: when we navigate to a preserved view, it first initializes the app. After creating the UI, the server navigates to localhost:8080/VAADIN/?v-r=init before it can actually navigate to the @PreseveOnRefresh view. The first navigation will clear all the cache because it doesn't have @PreserveOnRefresh.

manolo

Reviewable status: 4 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @manolo, @platosha, @qtdzz, and @vlukashov)

flow-client/src/main/resources/META-INF/resources/frontend/Flow.ts, line 63 at r1 (raw file):

Previously, platosha (Anton Platonov) wrote…

any does not give much sense.

Do we have a use case for this method’s return value? If not, should it be declared as Promise<void> perhaps?

Maybe even omit return here:
async start() {
  await this.flowInit();
  document.body.appendChild(this.container);
  this.flowNavigate(/*...*/);
}
TypeScript will infer Promise<void> then.

Done.

flow-client/src/main/resources/META-INF/resources/frontend/Flow.ts, line 65 at r1 (raw file):

Previously, qtdzz (Tien Nguyen) wrote…

I had a discussion with @vlukashov, we should put the content of server-side in the body instead of the container. So, the result will be closer to application bootstrapped by server-side.

Done.

flow-client/src/main/resources/META-INF/resources/frontend/Flow.ts, line 66 at r1 (raw file):

Previously, qtdzz (Tien Nguyen) wrote…

This also means the first start will take two round trips, right? I wonder if we could somehow modify the init request to include information for the first route so that we only need one round trip for initializing the app and loading the first view.

Yes, there was two round-trips.
Changed server side so as during the init request a location string can be passed.

flow-server/src/main/java/com/vaadin/flow/server/communication/JavaScriptBootstrapHandler.java, line 140 at r1 (raw file):

Previously, qtdzz (Tien Nguyen) wrote…

I investigated the failed test ClientIndexHandlerIT.should_preserveView_When_reloadPreservedOnRefreshView. It turns out that this change causes the error. What happened is: when we navigate to a preserved view, it first initializes the app. After creating the UI, the server navigates to localhost:8080/VAADIN/?v-r=init before it can actually navigate to the @PreseveOnRefresh view. The first navigation will clear all the cache because it doesn't have @PreserveOnRefresh.

Good catch. Added back the method which only initializes the route during the init phase when passing an extra location parameter.

qtdzz

Reviewed 8 of 8 files at r2.
Reviewable status: 6 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @manolo and @platosha)

flow-server/src/main/java/com/vaadin/flow/component/internal/JavaScriptBootstrapUI.java, line 221 at r2 (raw file):

                ui.getElement().removeAllChildren();
                ui.getElement().appendChild(newRoot.getElement());
                return;

I think we should call UIInternalUpdater.super.updateRoot(ui, oldRoot, newRoot); here, otherwise, it won't be able to reuse instances, e.g. Layout instances.

flow-server/src/main/java/com/vaadin/flow/server/communication/JavaScriptBootstrapHandler.java, line 145 at r2 (raw file):

            try {
                route = URLDecoder.decode(route, "UTF-8").replaceFirst("^/+", "");
            } catch (UnsupportedEncodingException e) {

Sonarqube probably will complain about this empty block.

flow-server/src/main/java/com/vaadin/flow/server/communication/JavaScriptBootstrapHandler.java, line 148 at r2 (raw file):

            Location location = new Location(route);
            ui.getRouter().initializeUI(ui, location);

Should we put this inside the try block?

manolo

Reviewable status: 6 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @manolo, @platosha, and @qtdzz)

flow-server/src/main/java/com/vaadin/flow/component/internal/JavaScriptBootstrapUI.java, line 221 at r2 (raw file):

Previously, qtdzz (Tien Nguyen) wrote…

                ui.getElement().removeAllChildren();
                ui.getElement().appendChild(newRoot.getElement());
                return;
I think we should call UIInternalUpdater.super.updateRoot(ui, oldRoot, newRoot); here, otherwise, it won't be able to reuse instances, e.g. Layout instances.

Nice, thanks. Done

flow-server/src/main/java/com/vaadin/flow/server/communication/JavaScriptBootstrapHandler.java, line 145 at r2 (raw file):

Previously, qtdzz (Tien Nguyen) wrote…

Sonarqube probably will complain about this empty block.

Done.

flow-server/src/main/java/com/vaadin/flow/server/communication/JavaScriptBootstrapHandler.java, line 148 at r2 (raw file):

Previously, qtdzz (Tien Nguyen) wrote…

            Location location = new Location(route);
            ui.getRouter().initializeUI(ui, location);

Should we put this inside the try block?

I don't think so, let don't shadow any error that might produce this code and let the server container to throw

qtdzz

Reviewed 3 of 4 files at r4, 1 of 1 files at r5.
Reviewable status: 3 unresolved discussions, 1 of 1 LGTMs obtained (waiting on @manolo and @platosha)

platosha

Reviewable status: 2 unresolved discussions, 2 of 1 LGTMs obtained (waiting on @manolo)

vaadin-bot · 2019-09-23T09:12:07Z

flow-server/src/main/java/com/vaadin/flow/server/communication/JavaScriptBootstrapHandler.java

@@ -123,9 +126,6 @@ protected BootstrapContext createAndInitUI(

        config.put(ApplicationConstants.SERVICE_URL, serviceUrl);

-        // Do not listen to pushState and routerLink events.
-        config.put(ApplicationConstants.APP_WC_MODE, true);
-
        // TODO(manolo) this comment is left intentionally because we


Complete the task associated to this TODO comment.

vaadin-bot · 2019-09-23T09:12:11Z

SonarQube analysis reported 1 issue

1 info

Watch the comments in this conversation to review them.

manolo

Dismissed @vaadin-bot from a discussion.
Reviewable status: complete! all discussions resolved, 2 of 1 LGTMs obtained

manolo added the hilla Issues related to Hilla label Sep 20, 2019

manolo requested a review from qtdzz September 20, 2019 08:23

manolo self-assigned this Sep 20, 2019

project-bot bot added this to External Reviews in OLD Vaadin Flow ongoing work (Vaadin 10+) Sep 20, 2019

qtdzz suggested changes Sep 20, 2019

View reviewed changes

platosha requested changes Sep 20, 2019

View reviewed changes

platosha reviewed Sep 20, 2019

View reviewed changes

qtdzz suggested changes Sep 20, 2019

View reviewed changes

manolo commented Sep 21, 2019

View reviewed changes

manolo added 4 commits September 23, 2019 08:28

CCDM: init router earlier to enable server-side routing

f4ee2b7

CCDM: implement flow.start() API (#6256)

8c5e806

CCDM: initialize UI in init request to avoid double round-trip

c08a53e

CCDM: client sends location in init request

7c97f84

manolo force-pushed the mcm/ccdm/flow-start branch from d29940f to 7c97f84 Compare September 23, 2019 06:28

ujoni added the +0.1.0 label Sep 23, 2019

qtdzz suggested changes Sep 23, 2019

View reviewed changes

manolo commented Sep 23, 2019

View reviewed changes

manolo force-pushed the mcm/ccdm/flow-start branch from ddf1793 to 13db7ad Compare September 23, 2019 08:32

CCDM: update body by calling updateRoot

3daf2f3

manolo force-pushed the mcm/ccdm/flow-start branch from 13db7ad to 3daf2f3 Compare September 23, 2019 08:33

qtdzz approved these changes Sep 23, 2019

View reviewed changes

platosha approved these changes Sep 23, 2019

View reviewed changes

OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from External Reviews to Reviewer approved Sep 23, 2019

vaadin-bot reviewed Sep 23, 2019

View reviewed changes

manolo commented Sep 23, 2019

View reviewed changes

manolo merged commit d527c98 into ccdm Sep 23, 2019

OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Reviewer approved to Done - pending release Sep 23, 2019

manolo deleted the mcm/ccdm/flow-start branch September 23, 2019 10:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CCDM: support server-side routing in client bootstrapping #6511

CCDM: support server-side routing in client bootstrapping #6511

manolo commented Sep 20, 2019 •

edited by mehdi-vaadin

qtdzz left a comment

platosha left a comment

platosha left a comment

qtdzz left a comment

manolo left a comment

qtdzz left a comment

manolo left a comment

qtdzz left a comment

platosha left a comment

vaadin-bot Sep 23, 2019

vaadin-bot commented Sep 23, 2019

manolo left a comment

CCDM: support server-side routing in client bootstrapping #6511

CCDM: support server-side routing in client bootstrapping #6511

Conversation

manolo commented Sep 20, 2019 • edited by mehdi-vaadin

qtdzz left a comment

Choose a reason for hiding this comment

platosha left a comment

Choose a reason for hiding this comment

platosha left a comment

Choose a reason for hiding this comment

qtdzz left a comment

Choose a reason for hiding this comment

manolo left a comment

Choose a reason for hiding this comment

qtdzz left a comment

Choose a reason for hiding this comment

manolo left a comment

Choose a reason for hiding this comment

qtdzz left a comment

Choose a reason for hiding this comment

platosha left a comment

Choose a reason for hiding this comment

vaadin-bot Sep 23, 2019

Choose a reason for hiding this comment

vaadin-bot commented Sep 23, 2019

manolo left a comment

Choose a reason for hiding this comment

manolo commented Sep 20, 2019 •

edited by mehdi-vaadin