Rewrite attach by @Id and template-in-template features

[denis-anisimov]

Fix for #3057, #3058.

---

This change is 

[vaadin-bot]

Refactor this method to reduce its Cognitive Complexity from 17 to the 15 allowed.

[vaadin-bot]

Remove this unused private "extractNodeId" method.

[vaadin-bot]

Immediately return this expression instead of assigning it to the temporary variable "domNode".

[vaadin-bot]

Suspicious comparison of Boolean references in com.vaadin.client.flow.StateNode.lambda$setDomNode$3(Function)

[caalador]

Reviewed 28 of 38 files at r1, 1 of 1 files at r2, 6 of 8 files at r3, 4 of 5 files at r5, 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 19 unresolved discussions.

---

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

                .set(domNodeSetListeners);
        copy.forEach(listener -> {
            if (listener.apply(this) == Boolean.TRUE) {

Should this just be listener.apply(this) as it already returns a boolean.

---

flow-client/src/main/java/com/vaadin/client/flow/binding/SimpleElementBindingStrategy.java, line 599 at r5 (raw file):

    }

    private void appendVirtualChild(BindingContext context, StateNode node,

Due to the complexity collecting to appendVirtualChild would it be a good idea/possible to move the virtual child binding parts to a util class or such? (not blocking)

---

flow-client/src/main/java/com/vaadin/client/flow/binding/SimpleElementBindingStrategy.java, line 709 at r7 (raw file):

        if (existingId != null) {
            failure = true;

This is not needed as we just return false if we come here.

---

flow-client/src/main/java/com/vaadin/client/flow/util/ClientJsonCodec.java, line 97 at r6 (raw file):

                int nodeId = (int) array.getNumber(1);
                Node domNode = tree.getNode(nodeId).getDomNode();
                return domNode;

How does ExecuteJavaScriptProcessor::handleInvocation manage if it receives a null value?

---

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

        // nothing has changed: no new child
        assertEquals(1, element.getChildElementCount());

Could also have a message that opens assert failure instead of the now uninformative 'expected: 1 actual: N'.
Same would go for all new asserts added to tests.

---

flow-server/src/main/java/com/vaadin/flow/nodefeature/VirtualChildrenList.java, line 80 at r6 (raw file):

        assert node != null;

        JsonObject object = Json.createObject();

The object name could be more descriptive elementDataObject or dataObject... (not blocking)

---

flow-server/src/main/java/com/vaadin/flow/nodefeature/VirtualChildrenList.java, line 86 at r6 (raw file):

        }

        node.getFeature(ElementData.class).setPyload(object);

Mind fixing setPyload() to setPayload()

---

flow-server/src/main/java/com/vaadin/server/communication/rpc/AttachTemplateChildRpcHandler.java, line 99 at r6 (raw file):

            }
        } else {
            logger.severe("Attach existing element request succeded");

This log marking should perhaps open a bit on why it's severe and add that no response should have come from the client.
Or drop the whole log line as the exception is thrown in any case.

---

flow-server/src/main/java/com/vaadin/ui/polymertemplate/TemplateInitializer.java, line 211 at r6 (raw file):

        Element element = new Element(tag);
        // TODO: should this be exposed as Element API ?

On both counts I would say that it should not be exposed in the API.
Some documentation for us would be good as we have some use cases where we need to get possible virtual children.

---

flow-server/src/test/java/com/vaadin/flow/nodefeature/VirtualChildrenListTest.java, line 41 at r6 (raw file):

    @Test
    public void insert_atIndexWithType_payloadIsSetAndElementIsInserted() {
        list.add(0, child, "foo", (String) null);

the String casts are redundant and not needed.

---

flow-tests/test-root-context/src/main/webapp/frontend/com/vaadin/flow/uitest/ui/template/JsGrandParent.html, line 32 at r6 (raw file):

        customElements.define(JsGrandTemplate.is, JsGrandTemplate);
    </script>
</dom-module>

new line

---

flow-tests/test-root-context/src/main/webapp/frontend/com/vaadin/flow/uitest/ui/template/JsSubTemplate.html, line 32 at r6 (raw file):

        customElements.define(JsSubTemplate.is, JsSubTemplate);
    </script>
</dom-module>

new line.

---

Comments from Reviewable

[denis-anisimov]

Review status: all files reviewed at latest revision, 12 unresolved discussions.

---

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

Previously, caalador wrote…

Should this just be listener.apply(this) as it already returns a boolean.

listener.apply(this) returns a Boolean.

Technically it may be null so this check is preferable than without ==.

But in reality I don't trust GWT compiler with its boxing/unboxing. So I prefer to use the exact type declared in the method return value (did you know e.g. that new Integer(1) doesn't equal new Integer(1) in GWT world ? so you can't use Integer as keys in a map)

---

flow-client/src/main/java/com/vaadin/client/flow/binding/SimpleElementBindingStrategy.java, line 599 at r5 (raw file):

Previously, caalador wrote…

Due to the complexity collecting to appendVirtualChild would it be a good idea/possible to move the virtual child binding parts to a util class or such? (not blocking)

I don't like utility classes because of logical spreading.
The logic is not that complex actually.
It just contains slightly unrelated blocks here which would have been extracted as methods.
But there is "break" logic inside them ( return statement) which prevents to improve this blocks.
It's the same with extracting to utility methods.

I would like to extract the logic out of the blocks but unfortunately it doesn't improve the code significantly.

---

flow-client/src/main/java/com/vaadin/client/flow/binding/SimpleElementBindingStrategy.java, line 709 at r7 (raw file):

Previously, caalador wrote…

This is not needed as we just return false if we come here.

Done.

---

flow-client/src/main/java/com/vaadin/client/flow/util/ClientJsonCodec.java, line 97 at r6 (raw file):

Previously, caalador wrote…

How does ExecuteJavaScriptProcessor::handleInvocation manage if it receives a null value?

The same as it had done previously : it should care about this itself.
It may happen only if the element is detached. And that's the developer responsibility to care about it.

This code has been introduced as a workaround for attach elements which are targets of JS execution.
Now such executions are deferred so the Dom node will be available at the moment when it's executed.

---

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

Previously, caalador wrote…

Could also have a message that opens assert failure instead of the now uninformative 'expected: 1 actual: N'.
Same would go for all new asserts added to tests.

Done.

---

flow-server/src/main/java/com/vaadin/flow/nodefeature/VirtualChildrenList.java, line 80 at r6 (raw file):

Previously, caalador wrote…

The object name could be more descriptive elementDataObject or dataObject... (not blocking)

Done.

---

flow-server/src/main/java/com/vaadin/flow/nodefeature/VirtualChildrenList.java, line 86 at r6 (raw file):

Previously, caalador wrote…

Mind fixing setPyload() to setPayload()

Done.

---

flow-server/src/main/java/com/vaadin/server/communication/rpc/AttachTemplateChildRpcHandler.java, line 99 at r6 (raw file):

Previously, caalador wrote…

This log marking should perhaps open a bit on why it's severe and add that no response should have come from the client.
Or drop the whole log line as the exception is thrown in any case.

Done.

---

flow-server/src/main/java/com/vaadin/ui/polymertemplate/TemplateInitializer.java, line 211 at r6 (raw file):

Previously, caalador wrote…

On both counts I would say that it should not be exposed in the API.
Some documentation for us would be good as we have some use cases where we need to get possible virtual children.

Done.

---

flow-server/src/test/java/com/vaadin/flow/nodefeature/VirtualChildrenListTest.java, line 41 at r6 (raw file):

Previously, caalador wrote…

the String casts are redundant and not needed.

No.
There are two overloaded methods add : one has a String as a parameter type, another has a JsonValue . So the compiler just doesn't know which one to invoke.

---

flow-tests/test-root-context/src/main/webapp/frontend/com/vaadin/flow/uitest/ui/template/JsGrandParent.html, line 32 at r6 (raw file):

Previously, caalador wrote…

new line

Done.

---

flow-tests/test-root-context/src/main/webapp/frontend/com/vaadin/flow/uitest/ui/template/JsSubTemplate.html, line 32 at r6 (raw file):

Previously, caalador wrote…

new line.

Done.

---

Comments from Reviewable

[vaadin-bot]

SonarQube analysis reported 7 issues

• 6 major
• 1 minor

Watch the comments in this conversation to review them.

3 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:

1. ExecuteJavaScriptProcessor.java#L166: Remove this unused private "handleRemoveExistingNode" method.
2. ExecuteJavaScriptProcessor.java#L175: Remove this unused private "getAppId" method.
3. SimpleElementBindingStrategy.java#L767: Remove this unused private "bindElementFromShadowRootByTagName" method.

[caalador]

Reviewed 9 of 9 files at r8.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

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

Previously, denis-anisimov (Denis) wrote…

listener.apply(this) returns a Boolean.

Technically it may be null so this check is preferable than without ==.

But in reality I don't trust GWT compiler with its boxing/unboxing. So I prefer to use the exact type declared in the method return value (did you know e.g. that new Integer(1) doesn't equal new Integer(1) in GWT world ? so you can't use Integer as keys in a map)

That's true, but should it then be Boolean.TRUE.equals(listener.apply(this)) don't have a great preference on this so it's fine as is.

---

flow-client/src/main/java/com/vaadin/client/flow/util/ClientJsonCodec.java, line 97 at r6 (raw file):

Previously, denis-anisimov (Denis) wrote…

The same as it had done previously : it should care about this itself.
It may happen only if the element is detached. And that's the developer responsibility to care about it.

This code has been introduced as a workaround for attach elements which are targets of JS execution.
Now such executions are deferred so the Dom node will be available at the moment when it's executed.

ok. Then it's fine.

---

Comments from Reviewable