Parse html import dependencies and add them as dependencies via UIInternals #3568

denis-anisimov · 2018-02-16T06:53:11Z

Fix for #3479

This change is

…ernals Fix for #3479

gilberto-torrezan · 2018-02-16T07:20:09Z

Reviewed 11 of 11 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

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

        private HtmlImportDependency(Collection<String> uris,
                LoadMode loadMode) {
            this.uris = uris;

You could store the URIs as unmodifiable collection right in the constructor, so you wouldn't need to build a new collection every time getUris() is called. Non blocking though, since the unmodifiable collection is just a wrapper.

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

             * Cannot happen in runtime.
             *
             * But not all unit tests set it. So let's just return the root uri

"return the root uri and return" ? What does it mean?

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

                .forEach(html -> html.getUris().forEach(
                        uri -> page.addHtmlImport(getHtmlImportValue(uri),
                                html.getLoadMode())));

This lambda inside lamba has become a bit ugly. Could refactor by using flatMap or extract to a method.

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

                .thenReturn(resolvedRoot);

        String importContent = "<link rel='import' href='relative1.html'>"

Should it consider closing tags (</link>) ?

flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/template/ThemedTemplateIT.java, line 78 at r1 (raw file):

                        "/frontend/com/vaadin/flow/uitest/ui/custom-theme/relative2.html"));
        Assert.assertTrue(
                "The themed HTML file for the absolute file"

Missing space between file and is.

Comments from Reviewable

caalador · 2018-02-16T07:30:20Z

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

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

                .getWrappedSession()).getHttpSession().getServletContext();

        String resolvedUri = factory.toServletContextPath(request, path);

What about webJars?

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

                parseHtmlImports(content, path)
                        .map(uri -> resolveUri(uri, path))
                        .forEach(uri -> parseDependencies(uri, dependencies));

Do we actually want to delve deeper and deeper based on the found links?

As now for a import containing a vaadin button reference (src version) we would delve and parse:

polymer/polymer-element.html
- lib/mixins/element-mixin.html
  - utils/boot.html
  - utils/settings.html
  - utils/mixin.html
  - utils/style-gather.html
  - utils/resolve-url.html
  - elements/dom-module.html
  - property-effects.html
  - properties-mixin.html
- lib/utils/html-tag.html
polymer/lib/mixins/gesture-event-listeners.html
vaadin-themable-mixin/vaadin-themable-mixin.html
vaadin-control-state-mixin/vaadin-control-state-mixin.html
vaadin-element-mixin/vaadin-element-mixin.html

etc. etc.
I would say that we should only look at the root template and let the others be.
Maximum would be if we could know that the link is a template that is "ours" then we could look through that also.

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

Previously, gilberto-torrezan (Gilberto Torrezan) wrote…

Should it consider closing tags (</link>) ?

<link> is not closed.
<link></link> is neither valid html4 nor valid html5 nor valid xhtml the behaviour of the browser is theoretically unexpected.

Comments from Reviewable

denis-anisimov · 2018-02-16T07:38:24Z

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

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

Previously, gilberto-torrezan (Gilberto Torrezan) wrote…

You could store the URIs as unmodifiable collection right in the constructor, so you wouldn't need to build a new collection every time getUris() is called. Non blocking though, since the unmodifiable collection is just a wrapper.

Done.

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

Previously, gilberto-torrezan (Gilberto Torrezan) wrote…

"return the root uri and return" ? What does it mean?

Outdated comment.
Done.

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

Previously, caalador wrote…

What about webJars?

What about them ?
Aren't they handled in the same way as a regular html import ?

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

Previously, caalador wrote…

Do we actually want to delve deeper and deeper based on the found links?

As now for a import containing a vaadin button reference (src version) we would delve and parse:

polymer/polymer-element.html

lib/mixins/element-mixin.html

utils/boot.html

utils/settings.html

utils/mixin.html

utils/style-gather.html

utils/resolve-url.html

elements/dom-module.html

property-effects.html

properties-mixin.html

lib/utils/html-tag.html

polymer/lib/mixins/gesture-event-listeners.html

vaadin-themable-mixin/vaadin-themable-mixin.html

vaadin-control-state-mixin/vaadin-control-state-mixin.html

vaadin-element-mixin/vaadin-element-mixin.html

etc. etc.
I would say that we should only look at the root template and let the others be.
Maximum would be if we could know that the link is a template that is "ours" then we could look through that also.

I don't know.
But I don't see a logical reason why we should not ......
Only if we don't want for some reasons.

Yes, in many cases it has no sense especially for the our web components where we just know that there is no reason to go deeply.

But why there can't be a situation when some file (it doesn't even have to be a component) imports other file which imports other file and all of them should be styled.

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

Previously, gilberto-torrezan (Gilberto Torrezan) wrote…

This lambda inside lamba has become a bit ugly. Could refactor by using flatMap or extract to a method.

flatMap doesn't work here unfortunately since the original html instance is needed to get its loadMode in the end.

flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/template/ThemedTemplateIT.java, line 78 at r1 (raw file):

Previously, gilberto-torrezan (Gilberto Torrezan) wrote…

Missing space between file and is.

Done.

Comments from Reviewable

vaadin-bot · 2018-02-16T07:42:19Z

flow-server/src/main/java/com/vaadin/flow/component/internal/HtmlDependencyParser.java

+
+            return parsedDocument.getElementsByTag("link").stream()
+                    .filter(link -> link.hasAttr("rel") && link.hasAttr("href"))
+                    .filter(link -> link.attr("rel").equals("import"))


Move the "import" string literal on the left side of this string comparison.

gilberto-torrezan · 2018-02-16T07:42:34Z

Reviewed 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

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

Previously, caalador wrote…

<link> is not closed.
<link></link> is neither valid html4 nor valid html5 nor valid xhtml the behaviour of the browser is theoretically unexpected.

Oddly enough, webcomponents.org uses <link></link>

Comments from Reviewable

caalador · 2018-02-16T07:42:55Z

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

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

Previously, denis-anisimov (Denis) wrote…

I don't know.
But I don't see a logical reason why we should not ......
Only if we don't want for some reasons.

Yes, in many cases it has no sense especially for the our web components where we just know that there is no reason to go deeply.

But why there can't be a situation when some file (it doesn't even have to be a component) imports other file which imports other file and all of them should be styled.

Then we should at least have a cache for checked as running through the polymer-element stack many times is a bit useless and will start creating lots of overhead (think bakery, storefront.html has that at least 5 times through refrences, probably even more)

Comments from Reviewable

caalador · 2018-02-16T07:46:01Z

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

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

Previously, denis-anisimov (Denis) wrote…

What about them ?
Aren't they handled in the same way as a regular html import ?

If you want to parse a html inside a webjar then you wont get it this way as the path is wrong.
see WebJarServer::hasWebJarResource that could be updated to actually return the stream it perhaps finds.

Comments from Reviewable

denis-anisimov · 2018-02-16T09:39:22Z

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks broke.

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

Previously, gilberto-torrezan (Gilberto Torrezan) wrote…

Oddly enough, webcomponents.org uses <link></link>

Is it really important?
This is the test and it works as expected. Parser is smart enough to parse this content properly.

Comments from Reviewable

caalador · 2018-02-16T12:09:19Z

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks broke.

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

Previously, caalador wrote…

If you want to parse a html inside a webjar then you wont get it this way as the path is wrong.
see WebJarServer::hasWebJarResource that could be updated to actually return the stream it perhaps finds.

One thing to note is that there is a chance that we don't have a WebJarServer which is why url translation validation was moved to VaadinServlet

Comments from Reviewable

denis-anisimov · 2018-02-16T12:59:32Z

Review status: 10 of 13 files reviewed at latest revision, 5 unresolved discussions.

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

Previously, caalador wrote…

One thing to note is that there is a chance that we don't have a WebJarServer which is why url translation validation was moved to VaadinServlet

Tests will be broken most likely, need jaavdocs, but does it work correctly now ?

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

Previously, caalador wrote…

Then we should at least have a cache for checked as running through the polymer-element stack many times is a bit useless and will start creating lots of overhead (think bakery, storefront.html has that at least 5 times through refrences, probably even more)

Will need to take care about this.

Comments from Reviewable

caalador · 2018-02-16T13:09:18Z

Reviewed 2 of 3 files at r3.
Review status: 12 of 13 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

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

Previously, denis-anisimov (Denis) wrote…

Tests will be broken most likely, need jaavdocs, but does it work correctly now ?

Yes this should now handle things as expected.

flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java, line 662 at r3 (raw file):

        InputStream inputStream = null;
        if (webJarServer != null) {

By default we usually first test servletContext and the webJars as servlet context is simpler.

flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java, line 667 at r3 (raw file):

        }
        if (inputStream == null) {
            inputStream = context.getResourceAsStream(resolvedUrl);

here you could just use getServletContext().getResourceAsStream(resolvedUrl) so you don't need to go through the messy steps with wrapped session.

Comments from Reviewable

denis-anisimov · 2018-02-16T14:47:16Z

Review status: 12 of 13 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java, line 662 at r3 (raw file):

Previously, caalador wrote…

By default we usually first test servletContext and the webJars as servlet context is simpler.

I've changed the logic to be able to gather URLs so that they can be cached.
It's easier to check first web jar server for the resource presence than servlet context since otherwise there will be a lot of checks and code just will be more complicated.

flow-server/src/main/java/com/vaadin/flow/server/VaadinServlet.java, line 667 at r3 (raw file):

Previously, caalador wrote…

here you could just use getServletContext().getResourceAsStream(resolvedUrl) so you don't need to go through the messy steps with wrapped session.

Done.

Comments from Reviewable

caalador · 2018-02-19T05:44:41Z

Reviewed 4 of 5 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from Reviewable

caalador · 2018-02-19T06:42:58Z

Reviewed 2 of 2 files at r6.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

vaadin-bot · 2018-02-19T06:43:01Z

SonarQube analysis reported 37 issues

4 critical
6 major
25 minor
2 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:

UIInternals.java#L542: Refactor this code to not nest more than 3 if/for/while/switch/try statements.
VaadinServlet.java#L146: Refactor this code to not nest more than 3 if/for/while/switch/try statements.
VaadinServlet.java#L539: Reduce the number of conditional operators (5) used in the expression (maximum allowed 3).
VaadinServlet.java#L612: Reduce the number of conditional operators (5) used in the expression (maximum allowed 3).
ComponentMetaData.java#L241: Reduce the total number of break and continue statements in this loop to use at most one.
UIInternals.java#L382: This block of commented-out lines of code should be removed.
UIInternals.java#L506: Refactor this method to reduce its Cognitive Complexity from 19 to the 15 allowed.
VaadinServlet.java#L1: Class com.vaadin.flow.server.VaadinServlet defines non-transient non-serializable instance field themeTranslations
VaadinServlet.java#L71: Remove this misleading mutable servlet instance field or make it "static" and/or "final"
VaadinServlet.java#L506: Merge this if statement with the enclosing one.

Denis Anisimov added 4 commits February 15, 2018 11:13

Parse html import dependencies and add them as dependencies via UIInt…

fcbdbbc

…ernals Fix for #3479

Correct absolute URL handling and add a test UI view for IT test

1bd88bf

Unit test for HtmlDependencyParser

9b2eaca

Update UI code for an IT test and make the IT test

5d4de31

Code review corrections.

2896c96

vaadin-bot reviewed Feb 16, 2018

View reviewed changes

Take into account webjars.

ac87170

Denis Anisimov added 3 commits February 16, 2018 20:34

Update unit test for the corrected code.

ae7e386

Unit tests

a184c3a

Log MalformedURLException

295b2b7

Code corrections

21a4c50

denis-anisimov merged commit 93b9932 into master Feb 19, 2018

denis-anisimov deleted the 3479-theming-imports branch February 19, 2018 07:21

denis-anisimov added this to the 1.0.0.alpha22 milestone Feb 22, 2018

This was referenced Apr 12, 2022

[Snyk] Upgrade @vaadin/vaadin-lumo-styles from 22.0.0-alpha9 to 22.0.9 #13496

Closed

[Snyk] Upgrade @vaadin/vaadin-text-field from 22.0.0-alpha9 to 22.0.9 #13497

Closed

This was referenced Apr 12, 2022

[Snyk] Upgrade @vaadin/vaadin-lumo-styles from 22.0.0-alpha9 to 22.0.9 #13498

Closed

[Snyk] Upgrade @vaadin/vaadin-lumo-styles from 22.0.0-alpha9 to 22.0.9 #13502

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse html import dependencies and add them as dependencies via UIInternals #3568

Parse html import dependencies and add them as dependencies via UIInternals #3568

denis-anisimov commented Feb 16, 2018 •

edited by SomeoneToIgnore

Loading

gilberto-torrezan commented Feb 16, 2018

caalador commented Feb 16, 2018

denis-anisimov commented Feb 16, 2018

vaadin-bot Feb 16, 2018

gilberto-torrezan commented Feb 16, 2018

caalador commented Feb 16, 2018

caalador commented Feb 16, 2018

denis-anisimov commented Feb 16, 2018

caalador commented Feb 16, 2018

denis-anisimov commented Feb 16, 2018

caalador commented Feb 16, 2018

denis-anisimov commented Feb 16, 2018

caalador commented Feb 19, 2018

caalador commented Feb 19, 2018

vaadin-bot commented Feb 19, 2018

Parse html import dependencies and add them as dependencies via UIInternals #3568

Parse html import dependencies and add them as dependencies via UIInternals #3568

Conversation

denis-anisimov commented Feb 16, 2018 • edited by SomeoneToIgnore Loading

gilberto-torrezan commented Feb 16, 2018

caalador commented Feb 16, 2018

denis-anisimov commented Feb 16, 2018

vaadin-bot Feb 16, 2018

Choose a reason for hiding this comment

gilberto-torrezan commented Feb 16, 2018

caalador commented Feb 16, 2018

caalador commented Feb 16, 2018

denis-anisimov commented Feb 16, 2018

caalador commented Feb 16, 2018

denis-anisimov commented Feb 16, 2018

caalador commented Feb 16, 2018

denis-anisimov commented Feb 16, 2018

caalador commented Feb 19, 2018

caalador commented Feb 19, 2018

vaadin-bot commented Feb 19, 2018

Top 10 extra issues

denis-anisimov commented Feb 16, 2018 •

edited by SomeoneToIgnore

Loading