Detect Push from WebComponentExporter classes #5369

denis-anisimov · 2019-03-28T12:28:18Z

Fix for #4984

This change is

Fix for #4984

ujoni

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

flow-server/src/main/java/com/vaadin/flow/server/webcomponent/WebComponentConfigurationRegistry.java, line 59 at r1 (raw file):

    private HashMap<String, WebComponentConfigurationImpl<? extends Component>> builderCache = new HashMap<>();

    private Map<Class<? extends Annotation>, Annotation> configuratoinAnnotations;

Type: configurationAnnotations

(HashMap would be serializable, to appease vaadin-bot)

flow-server/src/main/java/com/vaadin/flow/server/webcomponent/WebComponentConfigurationRegistry.java, line 205 at r1 (raw file):

     * <p>
     * {@link WebComponentExporter} classes may have
     *

Lacks @param.

Also, it returns configuration annotation for all the web components, so the Javadoc is a little misleading

flow-server/src/main/java/com/vaadin/flow/server/webcomponent/WebComponentConfigurationRegistry.java, line 208 at r1 (raw file):

     * @return
     */
    public <T extends Annotation> Optional<T> getConfigurationAnnotation(

Since "configuration" tends to refer to a singular web component configuration in the registry's domain, this should probably be renamed to something like getConfiguredAnnotation or something. Although, the wrapping annotation is called ConfigurationAnnotations. I'd like to call it something different, since it is set for the registry. But no better names come to mind. ConfigurableAnnotations? (no.)

flow-server/src/main/java/com/vaadin/flow/server/webcomponent/WebComponentConfigurationRegistry.java, line 278 at r1 (raw file):

        assert configurationLock.isHeldByCurrentThread();

        if (configuratoinAnnotations != null) {

Is this OSGi compliant? We might come here multiple times as bundles are loaded or unloaded. Seems like this would break.

Should we instead set configurationAnnotations to null, when we come here?

flow-server/src/main/java/com/vaadin/flow/server/webcomponent/WebComponentConfigurationRegistry.java, line 295 at r1 (raw file):

    }

    private void addConfigurationAnnottion(

Typo: addConfigurationAnnotation

flow-server/src/main/java/com/vaadin/flow/server/webcomponent/WebComponentConfigurationRegistry.java, line 307 at r1 (raw file):

            if (annotation != null && !annotation.equals(exporterAnnotation)) {
                throw new IllegalStateException(String.format(
                        "Different annotations of type '%s' are declared for the web component exporters: %s, %s",

I think "-- declared by the web --" would be better. Otherwise it seems that those annotations are exporters.

flow-server/src/test/java/com/vaadin/flow/server/communication/WebComponentProviderTest.java, line 203 at r1 (raw file):

    @Test
    public void notInitializedREgistry_themeIsEmpty() {

Type: notInitializedRegistry_themeIsEmpty

flow-tests/test-web-components/src/main/java/com/vaadin/flow/webcomponent/PushWebComponentServlet.java, line 25 at r1 (raw file):

@WebServlet(urlPatterns = { "/vaadin-push/*" }, initParams = {
        // @WebInitParam(name = "pushMode", value = "automatic"),
        @WebInitParam(name = "pushURL", value = "/vaadin-push") })

Was it so that #5061 will get rid of this row?

flow-server/src/test/java/com/vaadin/flow/server/startup/WebComponentExporterAwareValidatorTest.java, line 201 at r1 (raw file):

    }

    private void assertNnon_linked_theme_throws(

Type? assert_non_linked_theme_throws

denis-anisimov

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

flow-server/src/main/java/com/vaadin/flow/server/communication/WebComponentBootstrapHandler.java, line 53 at r1 (raw file):

BootstrapContext

flow-server/src/main/java/com/vaadin/flow/server/webcomponent/WebComponentConfigurationRegistry.java, line 59 at r1 (raw file):

Previously, ujoni (Joni) wrote…

Type: configurationAnnotations

(HashMap would be serializable, to appease vaadin-bot)

Done.

flow-server/src/main/java/com/vaadin/flow/server/webcomponent/WebComponentConfigurationRegistry.java, line 205 at r1 (raw file):

Previously, ujoni (Joni) wrote…

Lacks @param.

Also, it returns configuration annotation for all the web components, so the Javadoc is a little misleading

Looks like I has been interrupted in the middle of writing the javadoc.

Not sure whether the main sentence is better now.

flow-server/src/main/java/com/vaadin/flow/server/webcomponent/WebComponentConfigurationRegistry.java, line 208 at r1 (raw file):

Previously, ujoni (Joni) wrote…

Since "configuration" tends to refer to a singular web component configuration in the registry's domain, this should probably be renamed to something like getConfiguredAnnotation or something. Although, the wrapping annotation is called ConfigurationAnnotations. I'd like to call it something different, since it is set for the registry. But no better names come to mind. ConfigurableAnnotations? (no.)

The confusion caused by the fact that configuration term is used for web component exporter configuration (getConfiguration).
I wasn't even aware of this since I've not read the whole class and modified only small piece of it.

I would say that getConfiguration should have name getWCExporterConfig (or any variation).

I don't think Configured is good word here.

Since this is really config but fora whole embedded application I renamed it to specify this fact.

flow-server/src/main/java/com/vaadin/flow/server/webcomponent/WebComponentConfigurationRegistry.java, line 278 at r1 (raw file):

Previously, ujoni (Joni) wrote…

Is this OSGi compliant? We might come here multiple times as bundles are loaded or unloaded. Seems like this would break.

Should we instead set configurationAnnotations to null, when we come here?

This is really a hard question.....

Application config annotations should not be set in the way as we make it here.
It should be 1:1 with the whole application instead of 1:M because it's 1:1 with exporter.

So it should be set once at the application startup and should not be changed anymore.
(as Pekka proposed there should be just a property in property file which sets the value for the application).
Dealing with annotations declared for exporters we have to do validation that the same annotation is used to make it 1:1 with app.

We are doing it wrong and that's why there is such question at all...

It's OK to replace Push config if it has changed but it's not that obvious with Theme.

OK, I will just replace the map to the new one.
And we should not set it to null of course.

flow-server/src/main/java/com/vaadin/flow/server/webcomponent/WebComponentConfigurationRegistry.java, line 295 at r1 (raw file):

Previously, ujoni (Joni) wrote…

Typo: addConfigurationAnnotation

Done.

flow-server/src/main/java/com/vaadin/flow/server/webcomponent/WebComponentConfigurationRegistry.java, line 307 at r1 (raw file):

Previously, ujoni (Joni) wrote…

I think "-- declared by the web --" would be better. Otherwise it seems that those annotations are exporters.

Done.

flow-server/src/test/java/com/vaadin/flow/server/communication/WebComponentProviderTest.java, line 203 at r1 (raw file):

Previously, ujoni (Joni) wrote…

Type: notInitializedRegistry_themeIsEmpty

Done.

flow-tests/test-web-components/src/main/java/com/vaadin/flow/webcomponent/PushWebComponentServlet.java, line 25 at r1 (raw file):

Previously, ujoni (Joni) wrote…

Was it so that #5061 will get rid of this row?

Which row ?
#5061 is for the pushURL, yes.
And I have still to keep it since it's not fixed.
But pushMode is now configured via @Push for the web exporter and it allows to test the functionality via removing this line.

flow-server/src/test/java/com/vaadin/flow/server/startup/WebComponentExporterAwareValidatorTest.java, line 201 at r1 (raw file):

Previously, ujoni (Joni) wrote…

Type? assert_non_linked_theme_throws

Done.

vaadin-bot · 2019-03-29T07:37:02Z

SonarQube analysis reported 21 issues

14 major
6 minor
1 info

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:

BootstrapHandler.java#L1340: Add a private constructor to hide the implicit public one.
WebComponentConfigurationRegistry.java#L1: Class com.vaadin.flow.server.webcomponent.WebComponentConfigurationRegistry defines non-transient non-serializable instance field embeddedAppAnnotations
WebComponentConfigurationRegistry.java#L75: Remove usage of generic wildcard type.
WebComponentConfigurationRegistry.java#L89: Remove usage of generic wildcard type.
WebComponentConfigurationRegistry.java#L236: Remove usage of generic wildcard type.
WebComponentConfigurationRegistry.java#L262: "servletContext" is a method parameter, and should not be used for synchronization.
WebComponentConfigurationRegistry.java#L320: Remove this unused method parameter "context".
WebComponentConfigurationRegistry.java#L387: Remove usage of generic wildcard type.
ClassesSerializableTest.java#L184: Define and throw a dedicated exception instead of using a generic one.
ClassesSerializableTest.java#L314: Refactor this method to reduce its Cognitive Complexity from 16 to the 15 allowed.

ujoni

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

flow-server/src/main/java/com/vaadin/flow/server/communication/WebComponentBootstrapHandler.java, line 53 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

BootstrapContext

ServletContext should be BootstrapContext?

flow-server/src/main/java/com/vaadin/flow/server/webcomponent/WebComponentConfigurationRegistry.java, line 205 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

Looks like I has been interrupted in the middle of writing the javadoc.

Not sure whether the main sentence is better now.

With the extra text, it seems just fine.

flow-server/src/main/java/com/vaadin/flow/server/webcomponent/WebComponentConfigurationRegistry.java, line 278 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

This is really a hard question.....

Application config annotations should not be set in the way as we make it here.
It should be 1:1 with the whole application instead of 1:M because it's 1:1 with exporter.

So it should be set once at the application startup and should not be changed anymore.
(as Pekka proposed there should be just a property in property file which sets the value for the application).
Dealing with annotations declared for exporters we have to do validation that the same annotation is used to make it 1:1 with app.

We are doing it wrong and that's why there is such question at all...

It's OK to replace Push config if it has changed but it's not that obvious with Theme.

OK, I will just replace the map to the new one.
And we should not set it to null of course.

Should we create a ticket to investigate how much things might break under OSGi, or how do we remember that there might be a problem? Or do we have a robust way to test for the potential OSGi issues?

flow-tests/test-web-components/src/main/java/com/vaadin/flow/webcomponent/PushWebComponentServlet.java, line 25 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

Which row ?
#5061 is for the pushURL, yes.
And I have still to keep it since it's not fixed.
But pushMode is now configured via @Push for the web exporter and it allows to test the functionality via removing this line.

Yeah, pushURL. Alright. Just wanted to make sure I am still on track for changes.

denis-anisimov

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

flow-server/src/main/java/com/vaadin/flow/server/communication/WebComponentBootstrapHandler.java, line 53 at r1 (raw file):

Previously, ujoni (Joni) wrote…

ServletContext should be BootstrapContext?

Sorry, I don't understand what you are referring to.

ujoni

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

flow-server/src/main/java/com/vaadin/flow/server/communication/WebComponentBootstrapHandler.java, line 53 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

Sorry, I don't understand what you are referring to.

I just wasn't sure what your original comment was about.

denis-anisimov

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

flow-server/src/main/java/com/vaadin/flow/server/communication/WebComponentBootstrapHandler.java, line 53 at r1 (raw file):

Previously, ujoni (Joni) wrote…

I just wasn't sure what your original comment was about.

Have no idea how the comment appeared here at all.
May be somehow via clicking accidentally via reviewable.

flow-server/src/main/java/com/vaadin/flow/server/webcomponent/WebComponentConfigurationRegistry.java, line 278 at r1 (raw file):

Previously, ujoni (Joni) wrote…

Should we create a ticket to investigate how much things might break under OSGi, or how do we remember that there might be a problem? Or do we have a robust way to test for the potential OSGi issues?

It's not a matter if breaking things .
It's a matter of the expected behavior.

I don't know how it should work.

And the reason why I don't know is initially incorrect approach with annotations.
Annotations are part of Java, so they can appear/disappear with bundles changes in OSGi container.

Embedded application configuration should not be a part of Java at all since the configuration belongs to the application and not to classes. So it should be done once.
Doing the configuration via annotations we already do a logic error which generates a big confusion in OSGi.

Previously the initial configuration stayed unchanged regardless of bundle load/reload/appearing/disappearing.
Now it changes along with changes in activated bundles.

Which way is more correct ? I don't know.

flow-tests/test-web-components/src/main/java/com/vaadin/flow/webcomponent/PushWebComponentServlet.java, line 25 at r1 (raw file):

Previously, ujoni (Joni) wrote…

Yeah, pushURL. Alright. Just wanted to make sure I am still on track for changes.

Done.

denis-anisimov

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

flow-server/src/main/java/com/vaadin/flow/server/webcomponent/WebComponentConfigurationRegistry.java, line 205 at r1 (raw file):

Previously, ujoni (Joni) wrote…

With the extra text, it seems just fine.

Your comment is blocking.

flow-tests/test-web-components/src/main/java/com/vaadin/flow/webcomponent/PushWebComponentServlet.java, line 25 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…

Done.

Your comment is blocking.

ujoni

Reviewable status: complete! all discussions resolved, 1 of 1 LGTMs obtained

@SInCE

* Send service URL to the client side for Web Components UI (#5031) Fixes #5044 Fixes #5027 * Remove dependency from data to html-components (#5050) Fixes #5025 * Clarify StreamResource JavaDoc (#4981) * 1.5-SNAPSHOT (#5055) * Fix flow-component-renderer for Chrome 72 (#5060) Fixes #5025, vaadin/vaadin-grid-flow#502 * Make the java doc return say the correct thing. (#5063) * Shortcuts: change click shortcut's default behavior (#5058) (#5065) * Deprecated @VaadinServletConfiguration (#5052) * Update chrome driver version (#5020) * Update byte buddy version (#5026) * Update byte buddy version Fixes #4956 * Fix version id in drivers (#5083) * Deprecate getBodyAttributes and replace it with getHtmlAttributes (#5019) Fixes #4765 * Add IT test for push in embedded web components (#5059) Fixes #5006 * Add m2e lifecycle mapping for maven-antrun-plugin (#5087) * Add m2e lifecycle mapping for maven-antrun-plugin * Catch Throwable during maps sync handling (#5077) * Shortcuts no longer work, when element is disabled (#5099) * Shortcuts no longer work, when element is disabled * Fix getChildCount to always use filter (#5101) (Fixes vaadin-grid-flow/issues/464) * Fix invalid null check (#5132) * Javadoc - Add a component already added to a parent. (#5137) * Document the fact that a component with a parent will be added to a new parent and removed from the previous one. * Added Thread.sleeps to make sure Enter key is registered (#5139) * flow-component-renderer: set focus-target attribute to the first focusable node (#5142) Fixes #4191 Defining focus-target is necessary for the grid cell delegating focus to the first focusable component within it when Enter key is used (tests available from another PR on vaadin-grid-flow repo) * Add some configuration for surefire plugin (#5144) * Add some configuration for surefire plugin Due to issues with tests run under Windows 10 we need to add the trimStackTrace and reuseForks configurations also to the surefire plugin * Also update the surefire and failsafe plugins. * Register push static web resources (#5112) * Avoid for..of loop in renderer (#5171) * Add DOM listeners before attaching the element (#5195) Fixes #5152 * Add a generic mechanism for passing values back to the server (#5093) This is an enabler for #1724 and #4925. Fixes #4927 * Register superclass in case of route target collision (#5218) Fixes #5114 * Introduce ValueChangeMode#LAZY, and #TIMEOUT (#4945) Introduces ValueChangeMode#LAZY, and #TIMEOUT for delayed value synchronization to the server and makes Input implement HasValueChangeMode. * Check the function presence before calling it (#5208) Check the function presence before calling it and take care about children property as well. Fixes #5206 * enable TrackMessageSizeInterceptor to deal with multiple messages in same response * test for _trackMessageSize responsible to handle multiple messages coming from a single response * refactor imports * refactor TrackMessageSize test * using IOUtils to read from InputStream * Test for Long Polling when multiple threads run within a short period * Enhanced code * refactor getFileContent to use full path to retrieve vaadinPush.js * Correct javadocs for DomEvent (#5215) Fixes #5211 * Update jsoup version (#5253) Fixes #4127 * Avoid indeterministic behavior in tests. (#5283) Set locale explicitly in the tests. Fixes #3500 * Handle empty attribute values properly in HTML element (#5255) Fixes #5220 * Update chrome version in tests (#5295) * Remove redundant words form javadocs (#5294) Fixes #2770 * Update chromedriver version (#5297) * Exporter-based embeddable web component impl. (#5260) WebComponent is currently named IWebComponent to avoid naming collisions with the existing classes - this will be fixed once the functionality has been transferred over to the new components. * Fixed a NPE problem in WebComponentConfigurationRegistry (#5304) * Implement open and setLocation method to Page. (#4869) (#5166) * Fix navigate back to main view don't work in FF and Safari . (#5113) (#5291) * Register a servlet automatically if WCs present (#5331) Fixes #5070 * EWC: push property updates to client (#5324) * Apply theme for web component exporters (#5341) Fix for #4984 * Only make banner the target of pointer events (#5343) Currently the prompt consists of a full width transparent `#pwa-ip` div which is preventing click in elements behind it, and a `.banner` div which should be clickable * WebComponent fireEvent implementation (#5340) Implements #5272 * Fixes #5351 (#5358) * Detect Push from WebComponentExporter classes (#5369), Fix for #4984 * Make exported web components work on hosts where https is proxied to http (#5374) * Update config and generated class for rich-text-editor (#4754) * Merge InstanceConfigurator with exporter (#5376) (Fixes #5332) Also: Ignore abstract class in exporters discovering logic * Use "create" semantic for the createComponent method (#5379) Fixes #4634 * Avoid "web context" in web component module generation (#5391) * Avoid "web context" in web component module generation Fixes #5382 * Correct unit test * Improve JS code * Extract web component config factory (#5403) Fixes #5402 * Correct generated template to work properly in production mode (#5421) * Generate web component module file in the maven plugin to be able to transpile it (#5419) * Initial preparation for web component module files generation * Generate web component module content and add it into the bundle for transpilation * Generate web component module content and add the generated file into the bundle file * Hide common generation method * Correct typos. * Merge master into 2.0 Resolve the merge conflicts. * Remove empty @SInCE javadocs * Remove unused class * Fix the test

Detect Push from WebComponentExporter classes

5fbc22a

Fix for #4984

denis-anisimov requested a review from ujoni March 28, 2019 12:28

denis-anisimov mentioned this pull request Mar 28, 2019

Current WebComponentExporter doesn't work with @Theme(value = Lumo.class, variant = Lumo.DARK) #5372

Closed

ujoni suggested changes Mar 28, 2019

View reviewed changes

denis-anisimov commented Mar 29, 2019

View reviewed changes

Rename global embedded app configuration accessor methods.

f8cc55b

ujoni approved these changes Mar 29, 2019

View reviewed changes

denis-anisimov commented Mar 29, 2019

View reviewed changes

ujoni reviewed Mar 29, 2019

View reviewed changes

denis-anisimov commented Mar 29, 2019

View reviewed changes

ujoni approved these changes Mar 29, 2019

View reviewed changes

ujoni merged commit 94a4e39 into master Mar 29, 2019

ujoni deleted the 5194-push-sswc branch March 29, 2019 08:16

denis-anisimov mentioned this pull request Mar 29, 2019

Update annotation validation in Spring add-on vaadin/spring#414

Merged

ujoni added this to the 1.5.0.alpha2 milestone Mar 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect Push from WebComponentExporter classes #5369

Detect Push from WebComponentExporter classes #5369

denis-anisimov commented Mar 28, 2019 •

edited by SomeoneToIgnore

Loading

ujoni left a comment

denis-anisimov left a comment

vaadin-bot commented Mar 29, 2019

ujoni left a comment

denis-anisimov left a comment

ujoni left a comment

denis-anisimov left a comment

denis-anisimov left a comment

ujoni left a comment

Detect Push from WebComponentExporter classes #5369

Detect Push from WebComponentExporter classes #5369

Conversation

denis-anisimov commented Mar 28, 2019 • edited by SomeoneToIgnore Loading

ujoni left a comment

Choose a reason for hiding this comment

denis-anisimov left a comment

Choose a reason for hiding this comment

vaadin-bot commented Mar 29, 2019

Top 10 extra issues

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

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 commented Mar 28, 2019 •

edited by SomeoneToIgnore

Loading