Parse Polymer 2 template insertion style #6909

denis-anisimov · 2019-11-11T14:24:37Z

This change is

ujoni

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

flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/BundleParser.java, line 88 at r1 (raw file):

([\\`|\\'|\\\"])

Should the contents be wrapped in character class ([...])? It seems like there is an or pattern, but that should not work inside of a character class. Same seems to apply to TEMPLATE_PATTERN.

flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/BundleParser.java, line 201 at r1 (raw file):

                    continue;
                }
                return templates.get(0);

If the last bits with the continues were replaced with something like

            Optional<Element> template = domModule.map(module -> {
                JsoupUtils.removeCommentsRecursively(domModule.get());
                Elements templates = domModule.get()
                        .getElementsByTag(TEMPLATE_TAG_NAME);
                return !templates.isEmpty() ? templates.get(0) : null;
            });

            if (template.isPresent()) {
                return template.get();
            }

this could be a normal else if branch without the while and double continues

flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/JsoupUtils.java, line 73 at r1 (raw file):

    }

}

new line missing

flow-server/src/test/resources/META-INF/VAADIN/config/no-html-template.json, line 13 at r1 (raw file):

	],
	"hash": "foo"
}

Missing new line.

Some of the indents are formatted with tabs while others are spaces.

denis-anisimov

Reviewable status: 6 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @ujoni)

flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/BundleParser.java, line 88 at r1 (raw file):

Previously, ujoni (Joni) wrote…

([\\`|\\'|\\\"])
Should the contents be wrapped in character class ([...])? It seems like there is an or pattern, but that should not work inside of a character class. Same seems to apply to TEMPLATE_PATTERN.

I'm not sure what you mean......
([\\|\'|\"])` has

parenthesis ()
[] which makes or between back-tick, ' , ".

So for me everything looks fine.
What do you mean by character class ?

ujoni

Reviewable status: 6 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)

flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/BundleParser.java, line 88 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

I'm not sure what you mean......
([\\|\'|\"])` has

parenthesis ()

[] which makes or between back-tick, ' , ".

So for me everything looks fine.
What do you mean by character class ?

Shouldn't it just read (\\`|\\'|\\\") or [`'\\"] instead of ([\\`|\\'|\\\"]). The [ and ] marks a character group (and could be thought to have an implicit | between the characters) . I am not actually sure why the quotes (other than ") need to be escaped at all. They aren't special characters, right?

denis-anisimov

Reviewable status: 6 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @ujoni)

flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/BundleParser.java, line 88 at r1 (raw file):

Previously, ujoni (Joni) wrote…

Shouldn't it just read (\\`|\\'|\\\") or [`'\\"] instead of ([\\`|\\'|\\\"]). The [ and ] marks a character group (and could be thought to have an implicit | between the characters) . I am not actually sure why the quotes (other than ") need to be escaped at all. They aren't special characters, right?

Hmm.....

I don't think so.

( is just a marker for group to reuse it later and reference as \\n (it's referenced later on in regexp as \\1).

[] is used for or.

So ([\\|\'|\"])looks correct for me : it a group which consist of one character ( or , or). back tick quote is used for multiline strings ( I suppose). If you have a short string which is one line then ( I think ) you may use"or'` .
But I don't actually know exactly. This is just a modification of the existing pattern/logic.
At least it works....

ujoni

Reviewable status: 6 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)

flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/BundleParser.java, line 88 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

Hmm.....

I don't think so.

( is just a marker for group to reuse it later and reference as \\n (it's referenced later on in regexp as \\1).

[] is used for or.

So ([\\|\'|\"])looks correct for me : it a group which consist of one character ( or , or). back tick quote is used for multiline strings ( I suppose). If you have a short string which is one line then ( I think ) you may use"or'` .
But I don't actually know exactly. This is just a modification of the existing pattern/logic.
At least it works....

Ah right, we use the () result. So no removing that. But the [\\`|\'|\"] will match exactly one `, ', ", or | since the | does not have special meaning inside of [ and ].

So I guess someone could break stuff with

return magicTemplateVariable || html`custom template here`;

denis-anisimov

Reviewable status: 6 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @ujoni)

flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/BundleParser.java, line 88 at r1 (raw file):

Previously, ujoni (Joni) wrote…

Ah right, we use the () result. So no removing that. But the [\\`|\'|\"] will match exactly one `, ', ", or | since the | does not have special meaning inside of [ and ].

So I guess someone could break stuff with
return magicTemplateVariable || html`custom template here`;

Aaaaah.
Alright....
May be you are right.
I have to check it.

ujoni

Reviewable status: 6 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)

flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/BundleParser.java, line 88 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

Aaaaah.
Alright....
May be you are right.
I have to check it.

Though, the above would not actually work in any case since the regex only accepts white space between return and html.
Well, anyways, I think the | is not needed inside the square brackets.

denis-anisimov

Dismissed @vaadin-bot from 2 discussions.
Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @ujoni)

flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/BundleParser.java, line 88 at r1 (raw file):

Previously, ujoni (Joni) wrote…

Though, the above would not actually work in any case since the regex only accepts white space between return and html.
Well, anyways, I think the | is not needed inside the square brackets.

Done.

flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/BundleParser.java, line 201 at r1 (raw file):

Previously, ujoni (Joni) wrote…

If the last bits with the continues were replaced with something like

            Optional<Element> template = domModule.map(module -> {
                JsoupUtils.removeCommentsRecursively(domModule.get());
                Elements templates = domModule.get()
                        .getElementsByTag(TEMPLATE_TAG_NAME);
                return !templates.isEmpty() ? templates.get(0) : null;
            });

            if (template.isPresent()) {
                return template.get();
            }

this could be a normal else if branch without the while and double continues

I've extracted the code into a separate method.
May be it can be simplified a bit but I don't see how I can get rid of while without making another assumption.

There is a test which contain two dom-modules assignments.
One dom-module is empty ( kind of invalid , doesn't contain any template) and another
is a valid one.
It's not possible to choose the correct dom-module without while .

flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/JsoupUtils.java, line 73 at r1 (raw file):

Previously, ujoni (Joni) wrote…

new line missing

Done.

flow-server/src/test/resources/META-INF/VAADIN/config/no-html-template.json, line 13 at r1 (raw file):

Previously, ujoni (Joni) wrote…

Missing new line.

Some of the indents are formatted with tabs while others are spaces.

Done.

ujoni

Reviewed 1 of 3 files at r2.
Reviewable status: complete! all discussions resolved, 1 of 1 LGTMs obtained (waiting on @ujoni)

vaadin-bot · 2019-11-19T07:31:34Z

flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/BundleParser.java

+                    "Found Polymer 2 style insertion as a Polymer 3 template content {}",
+                    group);
+
+            templateDocument = Jsoup.parse(group);


Introduce a new variable instead of reusing the parameter "templateDocument".

vaadin-bot · 2019-11-19T07:31:35Z

flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/BundleParser.java

                .forEach(template::appendChild);

        return template;
    }

+    private static Element tryParsePolymer2(Document templateDocument,
+            Matcher noTemplateMatcher) {
+        while (noTemplateMatcher.find()


Reduce the total number of break and continue statements in this loop to use at most one.

vaadin-bot · 2019-11-19T07:31:38Z

SonarQube analysis reported 2 issues

1 major
1 minor

Watch the comments in this conversation to review them.

denis-anisimov

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

Fixes #6116 (cherry picked from commit 433206f)

mehdi-vaadin · 2019-11-25T12:59:01Z

Cherry-picked to 2.0.

Fixes #6116 (cherry picked from commit 433206f)

Denis Anisimov added 2 commits November 11, 2019 17:24

Parse Polymer 2 template insertion style

cc019e8

Fixes #6116

Use unique hash to prevent caching.

b4fac8d

ujoni added the +0.0.1 label Nov 12, 2019

ujoni suggested changes Nov 18, 2019

View reviewed changes

denis-anisimov commented Nov 18, 2019

View reviewed changes

ujoni reviewed Nov 18, 2019

View reviewed changes

denis-anisimov commented Nov 18, 2019

View reviewed changes

ujoni reviewed Nov 18, 2019

View reviewed changes

denis-anisimov commented Nov 18, 2019

View reviewed changes

ujoni reviewed Nov 18, 2019

View reviewed changes

denis-anisimov commented Nov 19, 2019

View reviewed changes

Fix some review and SQ comments

dc84134

ujoni approved these changes Nov 19, 2019

View reviewed changes

vaadin-bot reviewed Nov 19, 2019

View reviewed changes

ujoni added +1.0.0 and removed +0.0.1 labels Nov 19, 2019

denis-anisimov commented Nov 19, 2019

View reviewed changes

denis-anisimov merged commit 433206f into master Nov 19, 2019

denis-anisimov deleted the 6116-npm-parser branch November 19, 2019 07:33

ujoni added +0.0.1 and removed +1.0.0 labels Nov 19, 2019

denis-anisimov added the to be cherry picked label Nov 21, 2019

denis-anisimov added this to the 2.2.0.alpha5 milestone Nov 21, 2019

mehdi-vaadin pushed a commit that referenced this pull request Nov 25, 2019

Parse Polymer 2 template insertion style (#6909)

dd81b6e

Fixes #6116 (cherry picked from commit 433206f)

mehdi-vaadin pushed a commit that referenced this pull request Nov 25, 2019

Parse Polymer 2 template insertion style (#6909)

432e4c3

Fixes #6116 (cherry picked from commit 433206f)

mehdi-vaadin pushed a commit that referenced this pull request Dec 4, 2019

Parse Polymer 2 template insertion style (#6909)

9813b98

Fixes #6116 (cherry picked from commit 433206f)

mehdi-vaadin removed the to be cherry picked label Dec 4, 2019

mehdi-vaadin added the cherry picked label Dec 4, 2019

mehdi-vaadin pushed a commit that referenced this pull request Dec 5, 2019

Parse Polymer 2 template insertion style (#6909)

cd47f88

Fixes #6116 (cherry picked from commit 433206f)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse Polymer 2 template insertion style #6909

Parse Polymer 2 template insertion style #6909

denis-anisimov commented Nov 11, 2019 •

edited by mehdi-vaadin

ujoni left a comment

denis-anisimov left a comment

ujoni left a comment

denis-anisimov left a comment

ujoni left a comment

denis-anisimov left a comment

ujoni left a comment

denis-anisimov left a comment

ujoni left a comment

vaadin-bot Nov 19, 2019

vaadin-bot Nov 19, 2019

vaadin-bot commented Nov 19, 2019

denis-anisimov left a comment

mehdi-vaadin commented Nov 25, 2019

Parse Polymer 2 template insertion style #6909

Parse Polymer 2 template insertion style #6909

Conversation

denis-anisimov commented Nov 11, 2019 • edited by mehdi-vaadin

ujoni left a comment

Choose a reason for hiding this comment

denis-anisimov left a comment

Choose a reason for hiding this comment

ujoni left a comment

Choose a reason for hiding this comment

denis-anisimov left a comment

Choose a reason for hiding this comment

ujoni left a comment

Choose a reason for hiding this comment

denis-anisimov left a comment

Choose a reason for hiding this comment

ujoni left a comment

Choose a reason for hiding this comment

denis-anisimov left a comment

Choose a reason for hiding this comment

ujoni left a comment

Choose a reason for hiding this comment

vaadin-bot Nov 19, 2019

Choose a reason for hiding this comment

vaadin-bot Nov 19, 2019

Choose a reason for hiding this comment

vaadin-bot commented Nov 19, 2019

denis-anisimov left a comment

Choose a reason for hiding this comment

mehdi-vaadin commented Nov 25, 2019

denis-anisimov commented Nov 11, 2019 •

edited by mehdi-vaadin