CCDM: Adding a JsInit Handler for JS bootstrapper #6127

manolo · 2019-07-30T14:19:12Z

Part of the #6133 this change adds a bootstrap-handler to be called from the flow-client javascript.

The handler creates the user session, and the flow UI, returning a JSON that is used in the browser to run the bootstrap script that configures the hosted page and loads the flow-client module.

This handler is required in CCDM projects because flow does not need to be initialized until the user actually navigates to a view provided by flow instead of a pure client view.

This change is

qtdzz

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

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

    @Override
    public boolean synchronizedHandleRequest(VaadinSession session, VaadinRequest request, VaadinResponse response) throws IOException {
        if (session.getService().getDeploymentConfiguration().isCompatibilityMode()) {

Do you think we should also put this condition in canHandleRequest?

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

    }

    private JsonObject getInitialUidl(UI ui) {

This method and others around here are duplicated with those in com.vaadin.flow.server.BootstrapHandler.BootstrapPageBuilder because of the way they were implemented. Do you think that we should worry about it or just accept it for now and refactor them later?

flow-server/src/test/java/com/vaadin/flow/server/MockVaadinServletService.java, line 19 at r2 (raw file):

import javax.servlet.ServletException;

This change seems unrelated but it's up to you

flow-server/src/test/java/com/vaadin/flow/server/MockVaadinSession.java, line 21 at r2 (raw file):

import java.util.concurrent.locks.ReentrantLock;

import com.vaadin.flow.server.VaadinService;

This change seems unrelated but it's up to you

manolo

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @qtdzz)

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

Previously, qtdzz (Tien Nguyen) wrote…

Do you think we should also put this condition in canHandleRequest?

Done.

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

Previously, qtdzz (Tien Nguyen) wrote…

This method and others around here are duplicated with those in com.vaadin.flow.server.BootstrapHandler.BootstrapPageBuilder because of the way they were implemented. Do you think that we should worry about it or just accept it for now and refactor them later?

Refactored. There were not part of the public API.

vaadin-bot · 2019-08-01T18:23:46Z

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: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @manolo and @qtdzz)

qtdzz

Reviewed 1 of 4 files at r3, 2 of 2 files at r4.
Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained (waiting on @caalador)

flow-server/src/main/java/com/vaadin/flow/server/communication/JsInitHandler.java, line 136 at r4 (raw file):

!VaadinSession.getCurrent().getService()

Minor: is it safer to use request.getService() instead ?

qtdzz

Reviewed 2 of 2 files at r5.
Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained (waiting on @caalador)

manolo

Dismissed @vaadin-bot from a discussion.
Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained (waiting on @caalador)

caalador

Reviewed 3 of 5 files at r1, 2 of 6 files at r2, 3 of 4 files at r3, 1 of 2 files at r4, 2 of 2 files at r5.
Reviewable status: 5 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @manolo)

flow-server/src/main/java/com/vaadin/flow/server/BootstrapHandler.java, line 723 at r5 (raw file):

        }

        /**

Moving these around makes no sense for the actual ticket. Please revert this

flow-server/src/main/java/com/vaadin/flow/server/communication/JsInitHandler.java, line 54 at r5 (raw file):

given for

needed to

flow-server/src/main/java/com/vaadin/flow/server/communication/JsInitHandler.java, line 57 at r5 (raw file):

is thought for CCDM projects whose `index.html` response does not 
have any bootstrap code

is for client driven projects where index.html does not contain bootstrap data.

flow-server/src/main/java/com/vaadin/flow/server/communication/JsInitHandler.java, line 58 at r5 (raw file):

but it's responsability of the `@vaadin/flow` client

Bootstrapping is the responsibility of @vaadin/flow client code

flow-server/src/main/java/com/vaadin/flow/server/communication/JsInitHandler.java, line 63 at r5 (raw file):

JsInitHandler

Would be good to have the info about what this is in the name so JsInitBootstrapHandler or JavaScriptBootsrtapHandleras we have for WebComponentBootstrapHandler
Also if JsInit correct or should it be more like ClientJsonBootstrapHandler as from the server we don't handle JS in the handler, but do return json.

manolo

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

flow-server/src/main/java/com/vaadin/flow/server/BootstrapHandler.java, line 723 at r5 (raw file):

Previously, caalador wrote…

Moving these around makes no sense for the actual ticket. Please revert this

Otherwise we need to copy this code in the other class, @qtdzz suggested not to repeat code which is a good practice.

This is private API and does not break anything

manolo

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

flow-server/src/main/java/com/vaadin/flow/server/BootstrapHandler.java, line 723 at r5 (raw file):

Previously, manolo (Manuel Carrasco Moñino) wrote…

Otherwise we need to copy this code in the other class, @qtdzz suggested not to repeat code which is a good practice.

This is private API and does not break anything

Take a look to first change in the PR, it was not modifiying this class, but repeated code

caalador

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

flow-server/src/main/java/com/vaadin/flow/server/BootstrapHandler.java, line 723 at r5 (raw file):

Previously, manolo (Manuel Carrasco Moñino) wrote…

Take a look to first change in the PR, it was not modifiying this class, but repeated code

I see only one change and that just moves the methods to another position.

manolo

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

flow-server/src/main/java/com/vaadin/flow/server/BootstrapHandler.java, line 723 at r5 (raw file):

Previously, caalador wrote…

I see only one change and that just moves the methods to another position.

It's moving from one class to another in the same file

manolo

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

flow-server/src/main/java/com/vaadin/flow/server/BootstrapHandler.java, line 723 at r5 (raw file):

Previously, manolo (Manuel Carrasco Moñino) wrote…

It's moving from one class to another in the same file

I meant BootstrapPageBuilder -> BootstrapHandler

caalador

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

flow-server/src/main/java/com/vaadin/flow/server/BootstrapHandler.java, line 723 at r5 (raw file):

Previously, manolo (Manuel Carrasco Moñino) wrote…

I meant BootstrapPageBuilder -> BootstrapHandler

800 line internal class is not ok... The internal classes should really be broken out at some point for less sanity loss.

manolo

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

flow-server/src/main/java/com/vaadin/flow/server/communication/JsInitHandler.java, line 54 at r5 (raw file):

Previously, caalador wrote…

given for
needed to

Done.

flow-server/src/main/java/com/vaadin/flow/server/communication/JsInitHandler.java, line 57 at r5 (raw file):

Previously, caalador wrote…

is thought for CCDM projects whose `index.html` response does not 
have any bootstrap code
is for client driven projects where index.html does not contain bootstrap data.

Done.

flow-server/src/main/java/com/vaadin/flow/server/communication/JsInitHandler.java, line 58 at r5 (raw file):

Previously, caalador wrote…

but it's responsability of the `@vaadin/flow` client
Bootstrapping is the responsibility of @vaadin/flow client code

Done.

flow-server/src/main/java/com/vaadin/flow/server/communication/JsInitHandler.java, line 63 at r5 (raw file):

Previously, caalador wrote…

JsInitHandler
Would be good to have the info about what this is in the name so JsInitBootstrapHandler or JavaScriptBootsrtapHandleras we have for WebComponentBootstrapHandler
Also if JsInit correct or should it be more like ClientJsonBootstrapHandler as from the server we don't handle JS in the handler, but do return json.

Done.

caalador

Reviewed 4 of 4 files at r6.
Reviewable status: complete! all discussions resolved, 1 of 1 LGTMs obtained

qtdzz

Reviewable status: complete! all discussions resolved, 2 of 1 LGTMs obtained

vaadin-bot · 2019-08-06T09:29:14Z

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

+
+        config.put(ApplicationConstants.SERVICE_URL, serviceUrl);
+
+        // TODO(manolo) this comment is left intentionally because we


Complete the task associated to this TODO comment.

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 Jul 30, 2019

manolo requested a review from qtdzz July 30, 2019 14:19

project-bot bot added this to Review in progress in OLD Vaadin Flow ongoing work (Vaadin 10+) Jul 30, 2019

manolo changed the title ~~CCDM: Adding a JsInit bootstrapper~~ CCDM: Adding a JsInit Handler for JS bootstrapper Jul 30, 2019

manolo force-pushed the mcm/ccdm/jsinit branch 2 times, most recently from 435c017 to de6213d Compare August 1, 2019 09:58

manolo self-assigned this Aug 1, 2019

qtdzz suggested changes Aug 1, 2019

View reviewed changes

manolo commented Aug 1, 2019

View reviewed changes

manolo force-pushed the mcm/ccdm/jsinit branch 2 times, most recently from b1356d3 to 7caa295 Compare August 1, 2019 17:43

manolo commented Aug 3, 2019

View reviewed changes

manolo force-pushed the ccdm branch 2 times, most recently from 7d6b40c to ec2ca61 Compare August 5, 2019 06:26

manolo added 3 commits August 5, 2019 14:33

CCDM: Adding a JsInit bootstrapper

a05f6f0

CCDM: Extend BootstrapHandler

3f0a71f

CCDM: adding tests for JsInitHandler

96eaa19

manolo force-pushed the mcm/ccdm/jsinit branch from 7caa295 to 30f2517 Compare August 5, 2019 12:34

manolo added the CCDM Review Needed label Aug 6, 2019

manolo requested a review from caalador August 6, 2019 07:06

qtdzz reviewed Aug 6, 2019

View reviewed changes

qtdzz previously approved these changes Aug 6, 2019

View reviewed changes

OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Review in progress to Reviewer approved Aug 6, 2019

manolo dismissed qtdzz’s stale review via 79cfc59 August 6, 2019 07:22

manolo force-pushed the mcm/ccdm/jsinit branch from 30f2517 to 79cfc59 Compare August 6, 2019 07:22

CCDM: Applying reviewer suggestions

3760773

manolo force-pushed the mcm/ccdm/jsinit branch from 79cfc59 to 3760773 Compare August 6, 2019 07:25

qtdzz previously approved these changes Aug 6, 2019

View reviewed changes

OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Review in progress to Reviewer approved Aug 6, 2019

manolo commented Aug 6, 2019

View reviewed changes

caalador requested changes Aug 6, 2019

View reviewed changes

OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Reviewer approved to Review in progress Aug 6, 2019

manolo commented Aug 6, 2019

View reviewed changes

caalador requested changes Aug 6, 2019

View reviewed changes

manolo commented Aug 6, 2019

View reviewed changes

caalador reviewed Aug 6, 2019

View reviewed changes

manolo dismissed qtdzz’s stale review via 9788a1c August 6, 2019 08:41

manolo commented Aug 6, 2019

View reviewed changes

CCDM: rename handler to a better name

6ee4a08

manolo force-pushed the mcm/ccdm/jsinit branch from 9788a1c to 6ee4a08 Compare August 6, 2019 08:58

OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Review in progress to Reviewer approved Aug 6, 2019

caalador approved these changes Aug 6, 2019

View reviewed changes

qtdzz approved these changes Aug 6, 2019

View reviewed changes

vaadin-bot reviewed Aug 6, 2019

View reviewed changes

manolo commented Aug 6, 2019

View reviewed changes

manolo merged commit 7950f3d into ccdm Aug 6, 2019

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

manolo deleted the mcm/ccdm/jsinit branch August 6, 2019 09:31

ujoni added the +1.0.0 label Sep 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CCDM: Adding a JsInit Handler for JS bootstrapper #6127

CCDM: Adding a JsInit Handler for JS bootstrapper #6127

manolo commented Jul 30, 2019 •

edited

qtdzz left a comment

manolo left a comment

vaadin-bot commented Aug 1, 2019

manolo left a comment

qtdzz left a comment

qtdzz left a comment

manolo left a comment

caalador left a comment

manolo left a comment

manolo left a comment

caalador left a comment

manolo left a comment

manolo left a comment

caalador left a comment

manolo left a comment

caalador left a comment

qtdzz left a comment

vaadin-bot Aug 6, 2019

manolo left a comment


		config.put(ApplicationConstants.SERVICE_URL, serviceUrl);

		// TODO(manolo) this comment is left intentionally because we

CCDM: Adding a JsInit Handler for JS bootstrapper #6127

CCDM: Adding a JsInit Handler for JS bootstrapper #6127

Conversation

manolo commented Jul 30, 2019 • edited

qtdzz left a comment

Choose a reason for hiding this comment

manolo left a comment

Choose a reason for hiding this comment

vaadin-bot commented Aug 1, 2019

manolo left a comment

Choose a reason for hiding this comment

qtdzz 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

caalador left a comment

Choose a reason for hiding this comment

manolo left a comment

Choose a reason for hiding this comment

manolo left a comment

Choose a reason for hiding this comment

caalador left a comment

Choose a reason for hiding this comment

manolo left a comment

Choose a reason for hiding this comment

manolo left a comment

Choose a reason for hiding this comment

caalador left a comment

Choose a reason for hiding this comment

manolo left a comment

Choose a reason for hiding this comment

caalador left a comment

Choose a reason for hiding this comment

qtdzz left a comment

Choose a reason for hiding this comment

vaadin-bot Aug 6, 2019

Choose a reason for hiding this comment

manolo left a comment

Choose a reason for hiding this comment

manolo commented Jul 30, 2019 •

edited