Allow to add a dependency dynamically via JS expression

[denis-anisimov]

Fixes #6577

---

This change is 

[joheriks]

Reviewed 3 of 5 files at r2, 3 of 5 files at r3, 6 of 6 files at r4, 1 of 1 files at r6.
Reviewable status: 4 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)

---

flow-client/src/main/java/com/vaadin/client/DependencyLoader.java, line 72 at r6 (raw file):

            onLoad(event);
        }
    };

This object looks identical to EAGER_RESOURCE_LOAD_LISTENER and it does not seem stateful. Could a single object be used? (Methods inlineDependency and loadEagerDependency both refer to EAGER_RESOURCE_LOAD_LISTENER, and it looks to me like inlineDependency, loadEagerDependency and loadDynamicImport could be consolidated into a single method).

---

flow-client/src/test-gwt/java/com/vaadin/client/GwtSuite.java, line 52 at r6 (raw file):

        suite.addTestSuite(GwtExecuteJavaScriptElementUtilsTest.class);
        suite.addTestSuite(GwtDependencyLoaderTest.class);
        suite.addTestSuite(GwtMessageHandler.class);

Should this beGwtMessageHandlerTest.class? Is there a superfluous test class (GwtMessageHandler)?

---

flow-server/src/main/java/com/vaadin/flow/component/page/Page.java, line 363 at r6 (raw file):

     * supposed to return a JavaScript {@code Promise}.
     * <p>
     * No any change will be applied on the client side until resulting Promise

Grammar: "No any change..." -> "No change ...". Promise could be wrapped in <code>...</code> or {@code ...}

---

flow-server/src/main/java/com/vaadin/flow/shared/ui/Dependency.java, line 101 at r6 (raw file):

    /**
     * Creates a new dependency of the given type, to be loaded using JS
     * expression which is supposed to return a Promise.

Minor: Promise could be wrapped in {@code ...} or <code>...</code> for clarity.

[denis-anisimov]

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

---

flow-client/src/main/java/com/vaadin/client/DependencyLoader.java, line 72 at r6 (raw file):

Previously, joheriks (Johannes Eriksson) wrote…

This object looks identical to EAGER_RESOURCE_LOAD_LISTENER and it does not seem stateful. Could a single object be used? (Methods inlineDependency and loadEagerDependency both refer to EAGER_RESOURCE_LOAD_LISTENER, and it looks to me like inlineDependency, loadEagerDependency and loadDynamicImport could be consolidated into a single method).

Very good points.
Though I didn't touch this pieces of code at all but it's worth to fix the parts which has not been changed in this PR at all.

---

flow-client/src/test-gwt/java/com/vaadin/client/GwtSuite.java, line 52 at r6 (raw file):

Previously, joheriks (Johannes Eriksson) wrote…

Should this beGwtMessageHandlerTest.class? Is there a superfluous test class (GwtMessageHandler)?

Arghhhh.
It was. It's an initial state. I've renamed it at some point.
Git merge also participated in this PR to make work on it terrible.

Oh my god.
It's even worse: there are both of them .
If was the worst PR ever.

Done.

---

flow-server/src/main/java/com/vaadin/flow/component/page/Page.java, line 363 at r6 (raw file):

Previously, joheriks (Johannes Eriksson) wrote…

Grammar: "No any change..." -> "No change ...". Promise could be wrapped in <code>...</code> or {@code ...}

Done.

[denis-anisimov]

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

---

flow-client/src/main/java/com/vaadin/client/DependencyLoader.java, line 72 at r6 (raw file):

Previously, denis-anisimov (Denis) wrote…

Very good points.
Though I didn't touch this pieces of code at all but it's worth to fix the parts which has not been changed in this PR at all.

Done.

[joheriks]

Reviewed 4 of 5 files at r7.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)

---

flow-client/src/test-gwt/java/com/vaadin/client/GwtMessageHandlerTest.java, line 160 at r7 (raw file):

        handler.handleJSON(object.cast());

        runDeferred(() -> {

Are the results of the assertions in the runDeferred blocks actually checked? I tried adding assertTrue(false); at this point but mvn verify still succeeded.

[denis-anisimov]

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

---

flow-client/src/test-gwt/java/com/vaadin/client/GwtMessageHandlerTest.java, line 160 at r7 (raw file):

Previously, joheriks (Johannes Eriksson) wrote…

Are the results of the assertions in the runDeferred blocks actually checked? I tried adding assertTrue(false); at this point but mvn verify still succeeded.

Yes, it's not executed in the test though it has been working for me.
Not anymore.

That will delay this freaking stuff for several days more.
Cool.

[manolo]

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

---

flow-client/src/test-gwt/java/com/vaadin/client/GwtMessageHandlerTest.java, line 160 at r7 (raw file):

Previously, denis-anisimov (Denis) wrote…

Yes, it's not executed in the test though it has been working for me.
Not anymore.

That will delay this freaking stuff for several days more.
Cool.

Not sure how this work, but IIUC, what you need to do here is to run the gwt test asynchronously, the pattern normally in GWT is to use the delayTestFinish() and finishTest() pattern (take a look to GwtApplicationConnectionTest

[denis-anisimov]

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

---

flow-client/src/test-gwt/java/com/vaadin/client/GwtMessageHandlerTest.java, line 160 at r7 (raw file):

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

Not sure how this work, but IIUC, what you need to do here is to run the gwt test asynchronously, the pattern normally in GWT is to use the delayTestFinish() and finishTest() pattern (take a look to GwtApplicationConnectionTest

!!!!!!!!
Thanks a lot !!
That's exactly what I was looking for .
I've already found an ugly workaround but now it has no sense since it may
be done properly.
It looks like I was blind when I've been trying to find those methods by myself and failed.

Thanks you very much.

[manolo]

Reviewed 1 of 5 files at r2, 2 of 5 files at r3, 4 of 6 files at r4, 4 of 5 files at r7, 1 of 1 files at r8.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @joheriks)

---

flow-client/src/test-gwt/java/com/vaadin/client/GwtMessageHandlerTest.java, line 160 at r7 (raw file):

Previously, denis-anisimov (Denis) wrote…

!!!!!!!!
Thanks a lot !!
That's exactly what I was looking for .
I've already found an ugly workaround but now it has no sense since it may
be done properly.
It looks like I was blind when I've been trying to find those methods by myself and failed.

Thanks you very much.

👍

[joheriks]

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