Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

OSGi support for Flow 2.x in npm mode #9187

Closed
wants to merge 17 commits into from
Closed

OSGi support for Flow 2.x in npm mode #9187

wants to merge 17 commits into from

Conversation

denis-anisimov
Copy link
Contributor

Fixes #9146

caalador
caalador previously approved these changes Oct 15, 2020
View reviewed changes
Copy link
Contributor

@caalador caalador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the compatibility mode override be dropped as we are adding stats and token support?

flow-server/src/main/java/com/vaadin/flow/server/DeploymentConfigurationFactory.java Outdated Show resolved Hide resolved
flow-server/src/main/java/com/vaadin/flow/server/DeploymentConfigurationFactory.java Show resolved Hide resolved
flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java Show resolved Hide resolved
vaadin-bot
vaadin-bot reviewed Oct 15, 2020
View reviewed changes
flow-server/src/main/java/com/vaadin/flow/server/DeploymentConfigurationFactory.java
Boolean.toString(false));
initParameters.setProperty(SERVLET_PARAMETER_ENABLE_DEV_SERVER,
Boolean.toString(false));
initParameters.setProperty(EXTERNAL_STATS_FILE,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR Remove this use of "EXTERNAL_STATS_FILE"; it is deprecated. rule

pleku
pleku reviewed Oct 16, 2020
View reviewed changes
Copy link
Contributor

@pleku pleku left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question.

Also it seems that having osgi-related code in FrontendUtils and DeploymentConfigurationFactory (very little in latter) is a bit of a code smell. No need to do anything for now, but I think overall it would be great if we structure the OSGi code somehow as something that is just plugged in as an "alternative implementation" when OSGi is used, instead that it is spread all over the code to handle certain things in an OSGi way. It is just something to think about

flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java Outdated Show resolved Hide resolved
@denis-anisimov
Copy link
Contributor Author

One question.

Also it seems that having osgi-related code in FrontendUtils and DeploymentConfigurationFactory (very little in latter) is a bit of a code smell. No need to do anything for now, but I think overall it would be great if we structure the OSGi code somehow as something that is just plugged in as an "alternative implementation" when OSGi is used, instead that it is spread all over the code to handle certain things in an OSGi way. It is just something to think about

I totally agree.
Already thought about this and created a ticket : #9185 as a first step.
Then we should have our internal service interface to get resources with two impl: for plain WAR and for OSGi

@vaadin-bot vaadin-bot added +1.0.0 and removed +0.1.0 labels Oct 22, 2020
Denis Anisimov added 7 commits November 2, 2020 09:42
Fix OSGi Lookup related issues
81a7e2d 
Fix OSGi related unit test one more time
Fix OSGi related unit test one more time
Correct the resource access for VaadinService
263c917
Extend SPI, provide javadocs some unit tests fixes
ea2b761 
Minor code corrections, javadocs
Correct unit test
Correct code
Add test for Lookup impl in OSGi fake context
9ff27d3 
Add unit test for transferring attributes to the servlet context.
chore: Add unit test for loading instantiators
test: add unit test for getting token file
test: add unit tests for FrontendUtils
test: add unit test for deferred context initializer execution
test: add test for delegating to lookup
test: add tests for LookupInitializer
test: add test resource
test: correct resources location and set bower mode if no token file
test: correct resource location
test: make it possible to run felix-jetty in npm mode
3a04c10
Make httpclient dependency optional for now. (#9262)
737a045
refactoring: move OSGi resource provider impl to flow-server (#9268)
12bd846 
OSGi ResourceProvider impl should be available as a service at the moment when Vaadin WAB register a servlet.
It's not possible without extra unclear config to make sure that the service is registered if it's in the flow-osgi : the bundle may be activated at any moment regardless of servlet registration.
The issue doesn't appear if the service is registered at the flow-bundle start phase.
pleku
pleku suggested changes Nov 2, 2020
View reviewed changes
flow-server/src/main/java/com/vaadin/flow/di/Lookup.java Outdated Show resolved Hide resolved
flow-server/src/main/java/com/vaadin/flow/di/Lookup.java Show resolved Hide resolved
flow-server/src/main/java/com/vaadin/flow/di/Lookup.java Show resolved Hide resolved
flow-server/src/main/java/com/vaadin/flow/server/BootstrapHandler.java Show resolved Hide resolved
flow-server/src/main/java/com/vaadin/flow/server/VaadinServletContext.java Show resolved Hide resolved
...rc/main/java/com/vaadin/flow/server/startup/ClassLoaderAwareServletContainerInitializer.java Show resolved Hide resolved
...rc/main/java/com/vaadin/flow/server/startup/ClassLoaderAwareServletContainerInitializer.java Show resolved Hide resolved
...-server/src/main/java/com/vaadin/flow/server/startup/DeferredServletContextInitializers.java Show resolved Hide resolved
...-server/src/main/java/com/vaadin/flow/server/startup/DeferredServletContextInitializers.java Show resolved Hide resolved
flow-server/src/main/java/com/vaadin/flow/server/osgi/OSGiAccess.java Show resolved Hide resolved
@denis-anisimov
Copy link
Contributor Author

Fixes for the current review round : #9289

refactor: review fixes for OSGi (#9289)
92c0dee 
* refactoring: review fixes

* chore: add a comment about Jar packaging in test-root-context module

* fix: ensureServletContext should be always called

* fix: fix review comments
pleku
pleku reviewed Nov 3, 2020
View reviewed changes
Copy link
Contributor

@pleku pleku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed up till 12th commit. (not approving since we don't know if this will land to 2.5 or not)

@vaadin vaadin deleted a comment from pleku Nov 3, 2020
@vaadin vaadin deleted a comment from denis-anisimov Nov 3, 2020
vaadin-bot
vaadin-bot reviewed Nov 9, 2020
View reviewed changes
flow-server/src/main/java/com/vaadin/flow/server/osgi/VaadinBundleTracker.java
}

private void registerClientResources(Bundle clientBundle) {
Hashtable<String, Object> properties = new Hashtable<>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAJOR Replace the synchronized class "Hashtable" by an unsynchronized one such as "HashMap". rule

@mshabarov @caalador
chore: update chrome expectation to 87 (#9445) (#9545)
df47fe4 
Co-authored-by: caalador <mikael.grankvist@gmail.com>
@mg-dev-fis
Copy link

I'm also waiting for fix as we are using OSGi bridged servlet.

@denis-anisimov
Copy link
Contributor Author

I'm also waiting for fix as we are using OSGi bridged servlet.

This is already done in the master branch ( V19 Platform , 6.0 Flow).
The status of this change is quite unknown for 2.5.

@mg-dev-fis
Copy link

I'm doing migration from V8 to V14 and hoped that updating flow to 2.5 will fix the issue.

chore: remove browser version verification (#9939)
378eb73 
avoid failures on every Chrome version update
@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 43 issues

  • CRITICAL 10 critical
  • MAJOR 16 major
  • MINOR 13 minor
  • INFO 4 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:

  1. CRITICAL BootstrapHandler.java#L122: Define a constant instead of duplicating this literal "//<![CDATA[\n" 3 times. rule
  2. CRITICAL BootstrapHandler.java#L123: Define a constant instead of duplicating this literal "//]]>" 3 times. rule
  3. CRITICAL VaadinServlet.java#L527: Reduce the number of conditional operators (5) used in the expression (maximum allowed 3). rule
  4. CRITICAL OSGiAccess.java#L402: Either log or rethrow this exception. rule
  5. MAJOR pom.xml#L20: Make this line start at column 9. rule
  6. MAJOR OSGiAccess.java#L298: Define and throw a dedicated exception instead of using a generic one. rule
  7. MAJOR OSGiAccess.java#L402: Catch Exception instead of Throwable. rule
  8. MAJOR VaadinBundleTracker.java#L265: Reduce the total number of break and continue statements in this loop to use at most one. rule
  9. MAJOR OSGiWebComponentConfigurationRegistryTest.java#L125: Remove this use of "Thread.sleep()". rule
  10. MINOR NpmTemplateParser.java#L93: Remove this use of "loadMode"; it is deprecated. rule

@pleku pleku changed the title Read token file and stats.json file as bundle resources instead of Classpath resources OSGi support for Flow 2.x in npm mode Feb 3, 2021
@pleku
Copy link
Contributor

pleku commented Mar 4, 2021

Closing for now as OSGi support will likely not be backported to v14 at all, or not like it was done here anyway as this is slightly different from how it works since v19.

@pleku pleku closed this Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pleku pleku pleku requested changes

@vaadin-bot vaadin-bot vaadin-bot left review comments

@caalador caalador caalador left review comments

Assignees
No one assigned
Labels
+1.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@denis-anisimov @mg-dev-fis @vaadin-bot @pleku @caalador @mshabarov