#6537 NpmTemplateParser can now be overridden

[mvysny]

Karibu-Testing needs to hook into the NpmTemplateParser loading mechanism so that
Polymer Template files can be loaded from filesystem as opposed from a webpack server.

The benefits of the change is already demonstrated in the Karibu-Testing library:

• The modified NpmTemplateParser class can be found here: https://github.com/mvysny/karibu-testing/blob/master/karibu-testing-v10/src/main/java/com/github/mvysny/kaributesting/v10/NpmTemplateParserCopy.java
• The overriding class can be found here: https://github.com/mvysny/karibu-testing/blob/master/karibu-testing-v10/src/main/kotlin/com/github/mvysny/kaributesting/v10/MockNpmTemplateParser.kt

---

This change is 

[CLAassistant]

All committers have signed the CLA.

[CLAassistant]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[denis-anisimov]

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

a discussion (no related file):
I don't think this is valuable PR for many reasons:

• NpmTemplateParser is a singleton. This change makes it not singleton. This is serious design change.
• I don't see a reason why we need not singleton NpmTemplateParser.
• There is already a way to use your own parser.

First two items are subject of discussion of course.
But the third item is the fact that makes this PR unnecessary.

The default CTOR for is:

public PolymerTemplate() {
        this(VaadinService.getCurrent().getDeploymentConfiguration()
                .isCompatibilityMode() ? DefaultTemplateParser.getInstance()
                        : NpmTemplateParser.getInstance(),
                VaadinService.getCurrent());
    }

But there is a protected CTOR:

 protected PolymerTemplate(TemplateParser parser, VaadinService service) {

Which allows you to provide any parser implementation that you want.
You just should use another CTOR for your templates.

Or do you have some issues with this approach ?
Do you need some functionality implemented in NpmTemplateParser ( to reuse existing methods) ?
Or there are existing components which already uses NpmTemplateParser and you can't change them ?

---

[mvysny]

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

a discussion (no related file):

Previously, denis-anisimov (Denis) wrote…

I don't think this is valuable PR for many reasons:

• NpmTemplateParser is a singleton. This change makes it not singleton. This is serious design change.
• I don't see a reason why we need not singleton NpmTemplateParser.
• There is already a way to use your own parser.

First two items are subject of discussion of course.
But the third item is the fact that makes this PR unnecessary.

The default CTOR for is:

public PolymerTemplate() {
        this(VaadinService.getCurrent().getDeploymentConfiguration()
                .isCompatibilityMode() ? DefaultTemplateParser.getInstance()
                        : NpmTemplateParser.getInstance(),
                VaadinService.getCurrent());
    }

But there is a protected CTOR:

 protected PolymerTemplate(TemplateParser parser, VaadinService service) {

Which allows you to provide any parser implementation that you want.
You just should use another CTOR for your templates.

Or do you have some issues with this approach ?
Do you need some functionality implemented in NpmTemplateParser ( to reuse existing methods) ?
Or there are existing components which already uses NpmTemplateParser and you can't change them ?

Thank you :) Unfortunately I don't think the CTOR approach is the best way to proceed, please allow me to explain.

The Karibu-Testing library needs to hook into the NpmTemplateParser, in order to allow PolymerTemplates to load templates from the filesystem from the frontend/ folder. By default NpmTemplateParser loads those templates from resources (production mode) or from webpack daemon, none of which is available when Karibu-Testing is running. Please see mvysny/karibu-testing#29 for more info, but this is the gist of it.

I believe users will not want to modify all of their template's constructors to use custom template parser just for the sake of Karibu-Testing. That unfortunately rules out the CTOR approach.

In order for the Karibu-Testing to hook into the loading mechanism of NpmTemplateParser, it needs to override NpmTemplateParser.getSourcesFromTemplate() and set this parser to NpmTemplateParser.INSTANCE, which is tricky to do on Java 8-11 (since I need to remove the 'final' modifier and set the field via reflection) and impossible to do on Java 12+ (since it's not possible to remove the 'final' modifier via reflection anymore).

Therefore, I would like to open a discussion. Currently it's impossible to customize NpmTemplateParser by different means than by changing the constructor of every PolymerTemplate class in your app (which could potentially be 100+ classes), which is definitely not ideal. That's why I'd propose to move the factory code of NpmTemplateParser someplace where it can be configured from one place only. My first proposal would be to add VaadinService.getTemplateParser() but I'm sure you can suggest way better place since you know the Vaadin codebase far better than I do.

---

[denis-anisimov]

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

a discussion (no related file):

Previously, mvysny (Martin Vysny) wrote…

Thank you :) Unfortunately I don't think the CTOR approach is the best way to proceed, please allow me to explain.

The Karibu-Testing library needs to hook into the NpmTemplateParser, in order to allow PolymerTemplates to load templates from the filesystem from the frontend/ folder. By default NpmTemplateParser loads those templates from resources (production mode) or from webpack daemon, none of which is available when Karibu-Testing is running. Please see mvysny/karibu-testing#29 for more info, but this is the gist of it.

I believe users will not want to modify all of their template's constructors to use custom template parser just for the sake of Karibu-Testing. That unfortunately rules out the CTOR approach.

In order for the Karibu-Testing to hook into the loading mechanism of NpmTemplateParser, it needs to override NpmTemplateParser.getSourcesFromTemplate() and set this parser to NpmTemplateParser.INSTANCE, which is tricky to do on Java 8-11 (since I need to remove the 'final' modifier and set the field via reflection) and impossible to do on Java 12+ (since it's not possible to remove the 'final' modifier via reflection anymore).

Therefore, I would like to open a discussion. Currently it's impossible to customize NpmTemplateParser by different means than by changing the constructor of every PolymerTemplate class in your app (which could potentially be 100+ classes), which is definitely not ideal. That's why I'd propose to move the factory code of NpmTemplateParser someplace where it can be configured from one place only. My first proposal would be to add VaadinService.getTemplateParser() but I'm sure you can suggest way better place since you know the Vaadin codebase far better than I do.

OK, now I understand the usecase.
Thanks.

I still don't want to change the singleton design here.
NpmTemplateParser is just an implementation for TemplateParser.

And the NpmTemplateParser in its current state is not an issue.
But the issue is the way how TemplateParser is applied (chosen).

It's a question of a proper SPI.
Instead of using SPI we just hardcoded the parsers.

I see two ways of moving to SPI approach ( I definitely don't like VaadinService.getTemplateParser()).

• We may use Java standard SPI mechanism (ServiceLoaders). But that won't work in some cases (e.g. in OSGI). This is already done in fact in some cases (like VaadinServiceInitListener ) but there is a way to use another SPI mechanism in case Java SPI doesn't work ( so there is a way to override this). I don't see how we can make overridable here.
• We already have Instantiator. That serves to SPI purposes.

I think the best way would be to use Instantiator (if it's possible).

I would go via two steps:

• to fix it ASAP (and have it even in 2.0 ) you may use method Instantiator::getOrCreate(TemplateParser.class). Method getOrCreate will need an adjustment specifically for TemplateParser.class. It should replicate logic for TemplateParser.class case :VaadinService.getCurrent().getDeploymentConfiguration() .isCompatibilityMode() ? DefaultTemplateParser.getInstance() : NpmTemplateParser.getInstance() (so it should care right away about two modes).
• the reason why : to keep binary compatibility with 2.0 /2.1.
• for the modern version we should add a dedicated Instantiator::getTemplateParser method with the corresponding implementation.
• to be able to override default template parser you will need to override the Instantiator in your project.
• in addition to that the implementation of Instantiator::getTemplateParser (and may be getOrCreate(TemplateParser.class) ) may use Java ServiceLoader SPI mechanism. So there will be no need to override the Instantiator (but if you are lacking Java ServiceLoader SPI then you may override Instantiator).

That's the plan.

---

Must have been a mistake

[mvysny]

I'm not sure I understand what course of action is suggested to be taken.

[pleku]

I'm not sure I understand what course of action is suggested to be taken.

Under the line I would go via two steps: the options are listed, basically the 2.0 and 2.1 compatible fix and then another that could be possible to include in newer 2.x releases. It is up to you which you'll choose.
#6825 (review)

[mvysny]

Ah, right, I understand. The 2.0 solution is not possible without breaking backward compatibility. Instantiator::getOrCreate is an interface and so I can't introduce specialized functionality for creating TemplateParser.class there. I could insert that into DefaultInstantiator, but all other instantiators (say Spring Instantiator) wouldn't inherit this functionality and would require modifications.

Creating a new function Instantiator::getTemplateParser with default impl is thus the way to go. However, Instantiator is often overridden by DI frameworks, while we only need to override TemplateParser for testing purposes. That's why I'd suggest to to not to have Instantiator::getTemplateParser but rather VaadinServletService::getTemplateParser().

[denis-anisimov]

SpringInstantiator is able to create a bean even if it's not "Spring aware"
https://github.com/vaadin/spring/blob/master/vaadin-spring/src/main/java/com/vaadin/flow/spring/SpringInstantiator.java#L104

The method createRouteTarget delegates directly to getOrCreate and the purpose of the createRouteTarget was to provide a way to create a bean regardless of its "manageability" (routes are often not manageable).
So add-ons should be able to use Instantiator::getOrCreate for any class without any correction.
Of course if there is a specific logic for createRouteTarget which doesn't delegate to getOrCreate and specific getOrCreate then yes, they need corrections.

I didn't get this argument :

However, Instantiator is often overridden by DI frameworks, while we only need to override TemplateParser for testing purposes. That's why I'd suggest to to not to have Instantiator::getTemplateParser but rather VaadinServletService::getTemplateParser()

Why VaadinServletService should be used for this ? Instantiator is the exactly mechanism introduced for this kind of requirement: to be able to make some sort of SPI.
Having anything like this in VaadinServletService just complicates the API.

[mvysny]

Why VaadinServletService should be used for this ?

I see two reasons. At VaadinService:264 I see Router being created via the new operator instead of via the Instantiator. That's why I thought that there are certain system components in Flow which contain no injectable parts itself (as opposed to views/routes) and would not benefit from being created via Instantiator. I thought TemplateParser to be one of them. In fact one can't even create instances of DefaultTemplateParser nor NpmTemplateParser since they're singletons (even though this PR relaxes NpmTemplateParser which allows for overriding and instantiation of this overridden class).

Another reason is a more pragmatic one: if I would need to hook into Instantiator::getTemplateParser then I would have to create my own delegate Instantiator which overrides Instantiator::getTemplateParser only and delegates all other calls to the original Instantiator. Compared to that, overriding VaadinServletService::getTemplateParser() is easier since I need to override VaadinServletService anyway.

However, if you'd still like to have Instantiator::getTemplateParser over VaadinServletService::getTemplateParser() just let me know and I'll modify the PR.

[denis-anisimov]

Well, router is important part of the application and framework relies on it seriously.
So there is some sense to have it in VaadinService.
I don't remember exactly but I may guess that Router has been made before Instantiator and even Router may be should in Instantiator , not VaadinService.

TemplateParser is very specific implementation detail. And it exists only because we are using Polymer template whose support may be dropped from the framework without any dramatic changes.
So I definitely prefer Instantiator over VaadinService.

The second reason is just about convenience. I agree that it's more convenient to do one step to achieve the goal but that has lower priority than semantical approach. And that may be considered as another issue : make it easier to implement a custom Instantiator (provide a good SPI mechanism for it)

So I vote for Instantiator which has been exactly design for such purposes.

[denis-anisimov]

Reviewed 1 of 3 files at r3.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @mvysny)

---

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

public TemplateParser getTemplateParser

As I explained in the discussion it's semantically inconvenient to have this method here
for the internal functionality which is not an impartible part of the framework.

[mvysny]

Fair enough, moved the function to the Instantiator.

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @mvysny)

---

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

public class NpmTemplateParser implements TemplateParser {

    private static TemplateParser INSTANCE = new NpmTemplateParser();

Done.

[denis-anisimov]

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

---

flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/NpmTemplateParser.java, line 68 at r4 (raw file):

protected 

Any specific reason why ?

Access is done using singleton method access.
So it's not needed here.
Do you plan to extend this class to avoid duplicate the same logic ?

---

flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/NpmTemplateParser.java, line 158 at r4 (raw file):

protected 

Any specific reason why ?

protected method needs javadocs

[mvysny]

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

---

flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/NpmTemplateParser.java, line 68 at r4 (raw file):

Previously, denis-anisimov (Denis) wrote…

protected 

Any specific reason why ?

Access is done using singleton method access.
So it's not needed here.
Do you plan to extend this class to avoid duplicate the same logic ?

Exactly. Karibu-Testing's MockNpmTemplateParser reuses a lot of code from NpmTemplateParser, so my mock class simply extends NpmTemplateParser.

---

flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/NpmTemplateParser.java, line 158 at r4 (raw file):

Previously, denis-anisimov (Denis) wrote…

protected 

Any specific reason why ?

protected method needs javadocs

Karibu-Testing's MockNpmTemplateParser needs to override getSourcesFromTemplate() to look up the template from the frontend/ directory on the local filesystem, as opposed to loading it via http from webpack.

I can add the javadoc, just a second.

[denis-anisimov]

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

---

flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/NpmTemplateParser.java, line 68 at r4 (raw file):

Previously, mvysny (Martin Vysny) wrote…

Exactly. Karibu-Testing's MockNpmTemplateParser reuses a lot of code from NpmTemplateParser, so my mock class simply extends NpmTemplateParser.

OK, but javadocs please for protected CTOR.
I know it's just dumb to write javadocs for a default CTOR but this a generic rule.
Please just add something here.

[denis-anisimov]

Reviewed 2 of 3 files at r7, 1 of 1 files at r8.
Reviewable status: complete! all discussions resolved, 1 of 1 LGTMs obtained

[denis-anisimov]

PolymerTemplateTest.noAnnotationTemplate test failed.

[denis-anisimov]

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