Add support for returning values from JavaScript #5285

Legioth · 2019-03-15T12:10:23Z

Related to #1724

This change is

caalador

Reviewed 16 of 16 files at r1.
Reviewable status: 5 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @Legioth)

flow-server/src/main/java/com/vaadin/flow/component/internal/UIInternals.java, line 146 at r1 (raw file):

     * A pending JavaScript result that can be sent to the client.
     */
    public static class PendingJavaScriptInvocation

Is there a god reason to add this inside uiinternals that's already a bit too heavy?
(Would throw JavaScriptInvocation out too)

flow-server/src/main/java/com/vaadin/flow/component/page/PendingJavaScriptResult.java, line 95 at r1 (raw file):

     *            or <code>null</code> to ignore errors
     */
    default <T> void then(Class<T> targetType,

Why then? It's straight from JavaScript and not Java that the target audience is most likely used to use.
CompletableFuture uses exceptionally(Function<Throwable,? extends T> fn) for the exception and thenAccept(Consumer<? super T> action)for handling the result.
This would though require that it would have a fluent api to add them.

flow-server/src/main/java/com/vaadin/flow/component/page/PendingJavaScriptResult.java, line 180 at r1 (raw file):

     *            or <code>null</code> to ignore errors
     */
    void then(SerializableConsumer<JsonValue> resultHandler,

then without default implementation should probably be at the top.

flow-server/src/main/java/com/vaadin/flow/component/page/PendingJavaScriptResult.java, line 213 at r1 (raw file):

        return toCompletableFuture(JsonValue.class);
    }
}

missing new line

flow-server/src/test/java/com/vaadin/flow/component/internal/PendingJavaScriptInvocationTest.java, line 239 at r1 (raw file):

    private void assertStringSuccess() {
        Assert.assertFalse(jsonSuccessConsumer.isCaptured());

These could have a message as now we only get expected false got true. Would help to see what was not false in the test.

Legioth

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

flow-server/src/main/java/com/vaadin/flow/component/internal/UIInternals.java, line 146 at r1 (raw file):

Previously, caalador wrote…

Is there a god reason to add this inside uiinternals that's already a bit too heavy?
(Would throw JavaScriptInvocation out too)

Done.

I wouldn't want to move JavaScriptInvocation as part of this PR since that class has been there from before and only receives quite limited changes here.

flow-server/src/main/java/com/vaadin/flow/component/page/PendingJavaScriptResult.java, line 95 at r1 (raw file):

Previously, caalador wrote…

Why then? It's straight from JavaScript and not Java that the target audience is most likely used to use.
CompletableFuture uses exceptionally(Function<Throwable,? extends T> fn) for the exception and thenAccept(Consumer<? super T> action)for handling the result.
This would though require that it would have a fluent api to add them.

CompletableFuture uses thenXyz because there are so many different overloads with slightly different semantics. Here, the number is quite limited so a simpler name should imho be sufficient and still clear enough.

The purpose of these methods is to avoid complexities with chaining that you'd get with a fluent API. One can use toCompletableFuture if one wants to do something that is best expressed by chaining.

flow-server/src/main/java/com/vaadin/flow/component/page/PendingJavaScriptResult.java, line 180 at r1 (raw file):

Previously, caalador wrote…

then without default implementation should probably be at the top.

I've ordered these methods to be in an order that would logical for callers. Whether any specific method has a default implementation is only relevant for implementors, which I see as a secondary concern in this case.

flow-server/src/main/java/com/vaadin/flow/component/page/PendingJavaScriptResult.java, line 213 at r1 (raw file):

Previously, caalador wrote…

missing new line

Done. (Shouldn't Sonar automatically complain about these?)

flow-server/src/test/java/com/vaadin/flow/component/internal/PendingJavaScriptInvocationTest.java, line 239 at r1 (raw file):

Previously, caalador wrote…

These could have a message as now we only get expected false got true. Would help to see what was not false in the test.

Done.

caalador

Reviewed 10 of 10 files at r2.
Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained

flow-server/src/main/java/com/vaadin/flow/component/internal/UIInternals.java, line 146 at r1 (raw file):

Previously, Legioth (Leif Åstrand) wrote…

Done.

I wouldn't want to move JavaScriptInvocation as part of this PR since that class has been there from before and only receives quite limited changes here.

That's fine as it's its own refactor.

flow-server/src/main/java/com/vaadin/flow/component/page/PendingJavaScriptResult.java, line 213 at r1 (raw file):

Previously, Legioth (Leif Åstrand) wrote…

Done. (Shouldn't Sonar automatically complain about these?)

probably. I feel our sonar is all over the place with the checks.

flow-server/src/test/java/com/vaadin/flow/component/internal/PendingJavaScriptInvocationTest.java, line 272 at r2 (raw file):

success consuemr

typo in consumer. not that important.

vaadin-bot · 2019-03-21T10:04:18Z

flow-server/src/main/java/com/vaadin/flow/component/page/Page.java

     */
    @FunctionalInterface
+    @Deprecated
    public interface ExecutionCanceler extends Serializable {


Do not forget to remove this deprecated code someday.

vaadin-bot · 2019-03-21T10:04:19Z

flow-server/src/main/java/com/vaadin/flow/component/page/PendingJavaScriptResult.java

+ * @author Vaadin Ltd
+ */
+@SuppressWarnings("deprecation")
+public interface PendingJavaScriptResult extends ExecutionCanceler {


Remove this use of "ExecutionCanceler"; it is deprecated.

Related to #1724

vaadin-bot · 2019-03-21T11:42:04Z

SonarQube analysis reported 20 issues

5 major
1 minor
14 info

Watch the comments in this conversation to review them.

Top 10 extra issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

Page.java#L74: Call "Optional#isPresent()" before accessing the value.
Page.java#L88: Call "Optional#isPresent()" before accessing the value.
Page.java#L406: Define and throw a dedicated exception instead of using a generic one.
Element.java#L1458: Make this anonymous inner class a lambda
Element.java#L1491: Make this anonymous inner class a lambda
UIInternals.java#L241: com.vaadin.flow.component.internal.UIInternals.getLastProcessedMessageHash() may expose internal representation by returning UIInternals.lastProcessedMessageHash
UIInternals.java#L258: com.vaadin.flow.component.internal.UIInternals.setLastProcessedClientToServerId(int, byte[]) may expose internal representation by storing an externally mutable object into UIInternals.lastProcessedMessageHash
Element.java#L130: Do not forget to remove this deprecated code someday.
Element.java#L553: Do not forget to remove this deprecated code someday.
Element.java#L1166: Do not forget to remove this deprecated code someday.

caalador

Reviewed 1 of 1 files at r3, 1 of 1 files at r4.
Reviewable status: 2 unresolved discussions, 1 of 1 LGTMs obtained (waiting on @Legioth)

caalador

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

This reverts commit 16b8198.

ZheSun88 added the node/npm/pnpm/bun label Mar 19, 2019

ZheSun88 requested a review from caalador March 19, 2019 06:21

ZheSun88 assigned caalador Mar 19, 2019

caalador requested changes Mar 19, 2019

View reviewed changes

caalador removed the node/npm/pnpm/bun label Mar 19, 2019

Legioth force-pushed the jsReturnValues branch from 93fedb1 to 1232f8a Compare March 21, 2019 09:11

Legioth commented Mar 21, 2019

View reviewed changes

caalador approved these changes Mar 21, 2019

View reviewed changes

Legioth force-pushed the jsReturnValues branch from 1232f8a to 28e84e7 Compare March 21, 2019 09:38

vaadin-bot reviewed Mar 21, 2019

View reviewed changes

Legioth force-pushed the jsReturnValues branch from 28e84e7 to 5e94403 Compare March 21, 2019 10:39

Add support for returning values from JavaScript

76a615e

Related to #1724

Legioth force-pushed the jsReturnValues branch from 5e94403 to 76a615e Compare March 21, 2019 11:18

caalador approved these changes Mar 21, 2019

View reviewed changes

caalador merged commit 16b8198 into 2.0 Mar 21, 2019

caalador deleted the jsReturnValues branch March 21, 2019 11:52

manolo mentioned this pull request Mar 21, 2019

branch 2.0 is broken after adding support for returning JS values #5325

Closed

manolo added a commit that referenced this pull request Mar 21, 2019

Revert "Add support for returning values from JavaScript (#5285)"

12a1d1e

This reverts commit 16b8198.

caalador added this to the 2.0.0.alpha3 milestone May 3, 2019

Legioth mentioned this pull request May 8, 2019

When calling a function defined in a webcomponent, I want to be able to receive the return value of the call, so I can build server-side logic that uses the returned value #1724

Closed

This was referenced Feb 10, 2023

[Snyk] Upgrade @vaadin/vaadin-themable-mixin from 23.3.0-alpha2 to 23.3.5 #15897

Closed

[Snyk] Upgrade @vaadin/vaadin-lumo-styles from 23.3.0-alpha2 to 23.3.5 #15898

Closed

[Snyk] Upgrade @vaadin/vaadin-text-field from 23.3.0-alpha2 to 23.3.5 #15899

Closed

Artur- mentioned this pull request Feb 10, 2023

[Snyk] Upgrade @vaadin/polymer-legacy-adapter from 23.3.0-alpha2 to 23.3.5 #15900

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for returning values from JavaScript #5285

Add support for returning values from JavaScript #5285

Legioth commented Mar 15, 2019 •

edited by SomeoneToIgnore

Loading

caalador left a comment

Legioth left a comment

caalador left a comment

vaadin-bot Mar 21, 2019

vaadin-bot Mar 21, 2019

vaadin-bot commented Mar 21, 2019

caalador left a comment

caalador left a comment

Add support for returning values from JavaScript #5285

Add support for returning values from JavaScript #5285

Conversation

Legioth commented Mar 15, 2019 • edited by SomeoneToIgnore Loading

caalador left a comment

Choose a reason for hiding this comment

Legioth left a comment

Choose a reason for hiding this comment

caalador left a comment

Choose a reason for hiding this comment

vaadin-bot Mar 21, 2019

Choose a reason for hiding this comment

vaadin-bot Mar 21, 2019

Choose a reason for hiding this comment

vaadin-bot commented Mar 21, 2019

Top 10 extra issues

caalador left a comment

Choose a reason for hiding this comment

caalador left a comment

Choose a reason for hiding this comment

Legioth commented Mar 15, 2019 •

edited by SomeoneToIgnore

Loading