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

fix: pin platform transitive dependencies with npm #10864

Merged
merged 5 commits into from
May 6, 2021
Merged

fix: pin platform transitive dependencies with npm #10864

merged 5 commits into from
May 6, 2021

Conversation

pleku
Copy link
Contributor

@pleku pleku commented Apr 30, 2021

Uses the same vaadin_versions.json file based pinning as with pnpm.
In case the application has @NpmPackage annotation with a certain version,
then that is used over the platform pinned version. When that is older,
a warning is logged (like with pnpm).
In case a dependency has been pinned directly in package.json, then that
is used over the platform pinned version. When that is older, a warning
is logged (like with pnpm).

Fixes #10572

@pleku pleku requested a review from caalador April 30, 2021 16:22
@Artur-
Copy link
Member

Artur- commented May 3, 2021

Trying to use this with 20.0-SNAPSHOT results in

npm ERR! code ETARGET
npm ERR! notarget No matching version found for @vaadin/vaadin-core@20.0-SNAPSHOT.
npm ERR! notarget In most cases you or one of your dependencies are requesting
npm ERR! notarget a package version that doesn't exist.

@web-padawan web-padawan mentioned this pull request May 4, 2021
Closed
@pleku
fix: pin platform transitive dependencies with npm
60661d5 
Uses the same vaadin_versions.json file based pinning as with pnpm.
In case the application has @NpmPackage annotation with a certain version,
then that is used over the platform pinned version. When that is older,
a warning is logged (like with pnpm).
In case a dependency has been pinned directly in package.json, then that
is used over the platform pinned version. When that is older, a warning
is logged (like with pnpm).

Fixes #10572
caalador
caalador requested changes May 4, 2021
View reviewed changes
flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendVersion.java Outdated Show resolved Hide resolved
flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendVersion.java Show resolved Hide resolved
flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendVersion.java Outdated Show resolved Hide resolved
flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendVersion.java Outdated Show resolved Hide resolved
flow-server/src/main/java/com/vaadin/flow/server/frontend/NodeUpdater.java Outdated Show resolved Hide resolved
flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskUpdatePackages.java Show resolved Hide resolved
flow-server/src/main/java/com/vaadin/flow/server/frontend/NodeUpdater.java Show resolved Hide resolved
@pleku
Copy link
Contributor Author

pleku commented May 4, 2021 •
edited

Trying to use this with 20.0-SNAPSHOT results in
npm ERR! code ETARGET
npm ERR! notarget No matching version found for @vaadin/vaadin-core@20.0-SNAPSHOT.
npm ERR! notarget In most cases you or one of your dependencies are requesting
npm ERR! notarget a package version that doesn't exist.

Still need to fix and add a test for this

EDIT - done

@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 14 issues

  • CRITICAL 4 critical
  • MAJOR 7 major
  • MINOR 2 minor
  • INFO 1 info

Top 10 issues

  1. CRITICAL FrontendUtils.java#L1157: Either log or rethrow this exception. rule
  2. CRITICAL NodeUpdater.java#L321: Either log or rethrow this exception. rule
  3. CRITICAL TaskRunNpmInstall.java#L294: Reduce the number of conditional operators (4) used in the expression (maximum allowed 3). rule
  4. CRITICAL TaskUpdatePackages.java#L149: Refactor this code to not nest more than 3 if/for/while/switch/try statements. rule
  5. MAJOR NodeTasks.java#L1: Class com.vaadin.flow.server.frontend.NodeTasks$Builder defines non-transient non-serializable instance field lookup rule
  6. MAJOR NodeTasks.java#L142: Make "lookup" transient or serializable. rule
  7. MAJOR TaskRunNpmInstall.java#L327: Refactor this method to reduce its Cognitive Complexity from 16 to the 15 allowed. rule
  8. MAJOR TaskUpdatePackages.java#L84: Constructor has 8 parameters, which is greater than 7 authorized. rule
  9. MAJOR TaskUpdatePackages.java#L124: Refactor this method to reduce its Cognitive Complexity from 20 to the 15 allowed. rule
  10. MAJOR TaskUpdatePackages.java#L176: Take the required action to fix the issue indicated by this comment. rule

@pleku pleku requested a review from caalador May 6, 2021 05:50
@pleku pleku merged commit ef7c5a3 into master May 6, 2021
@pleku pleku deleted the fix/10572 branch May 6, 2021 06:15
vaadin-bot pushed a commit that referenced this pull request May 6, 2021
@pleku @vaadin-bot
fix: pin platform transitive dependencies with npm (#10864)
a8deca0 
Uses the same vaadin_versions.json file based pinning as with pnpm.
In case the application has @NpmPackage annotation with a certain version,
then that is used over the platform pinned version. When that is older,
a warning is logged (like with pnpm).
In case a dependency has been pinned directly in package.json, then that
is used over the platform pinned version. When that is older, a warning
is logged (like with pnpm).

Fixes #10572
vaadin-bot pushed a commit that referenced this pull request May 6, 2021
@pleku @vaadin-bot
fix: pin platform transitive dependencies with npm (#10864)
1de16f9 
Uses the same vaadin_versions.json file based pinning as with pnpm.
In case the application has @NpmPackage annotation with a certain version,
then that is used over the platform pinned version. When that is older,
a warning is logged (like with pnpm).
In case a dependency has been pinned directly in package.json, then that
is used over the platform pinned version. When that is older, a warning
is logged (like with pnpm).

Fixes #10572
pleku added a commit that referenced this pull request May 7, 2021
@vaadin-bot @pleku
fix: pin platform transitive dependencies with npm (#10864) (#10920)
01193c1 
Uses the same vaadin_versions.json file based pinning as with pnpm.
In case the application has @NpmPackage annotation with a certain version,
then that is used over the platform pinned version. When that is older,
a warning is logged (like with pnpm).
In case a dependency has been pinned directly in package.json, then that
is used over the platform pinned version. When that is older, a warning
is logged (like with pnpm).

Fixes #10572

Co-authored-by: Pekka Hyvönen <pekka@vaadin.com>
Artur- pushed a commit that referenced this pull request May 18, 2021
@pleku @Artur-
fix: pin platform transitive dependencies with npm (#10864)
cb05b83 
Uses the same vaadin_versions.json file based pinning as with pnpm.
In case the application has @NpmPackage annotation with a certain version,
then that is used over the platform pinned version. When that is older,
a warning is logged (like with pnpm).
In case a dependency has been pinned directly in package.json, then that
is used over the platform pinned version. When that is older, a warning
is logged (like with pnpm).

Fixes #10572
@pleku pleku mentioned this pull request Aug 12, 2021
Merged
@pleku
Copy link
Contributor Author

pleku commented Aug 12, 2021

This wasn't picked to 14-series, but to be honest, I don't remember why it was not - my bet is that due to "caution" it was to be tested in 7.0 (v20) first, where it seems to be working out fine.

pleku added a commit that referenced this pull request Aug 12, 2021
@pleku
fix: pin platform trans. deps with npm CP: #10864
124cbb1 
Uses the same vaadin_versions.json file based pinning as with pnpm.
In case the application has @NpmPackage annotation with a certain version,
then that is used over the platform pinned version. When that is older,
a warning is logged (like with pnpm).
In case a dependency has been pinned directly in package.json, then that
is used over the platform pinned version. When that is older, a warning
is logged (like with pnpm).

Fixes #10572
pleku added a commit that referenced this pull request Aug 17, 2021
@pleku
fix: pin platform trans. deps with npm CP: #10864
b155f2d 
Uses the same vaadin_versions.json file based pinning as with pnpm.
In case the application has @NpmPackage annotation with a certain version,
then that is used over the platform pinned version. When that is older,
a warning is logged (like with pnpm).
In case a dependency has been pinned directly in package.json, then that
is used over the platform pinned version. When that is older, a warning
is logged (like with pnpm).

Fixes #10572
pleku added a commit that referenced this pull request Aug 17, 2021
@pleku
fix: pin platform trans. deps with npm CP: #10864 (#11567)
31af267 
Uses the same vaadin_versions.json file based pinning as with pnpm.
In case the application has @NpmPackage annotation with a certain version,
then that is used over the platform pinned version. When that is older,
a warning is logged (like with pnpm).
In case a dependency has been pinned directly in package.json, then that
is used over the platform pinned version. When that is older, a warning
is logged (like with pnpm).

Fixes #10572
@taefi taefi mentioned this pull request Jun 22, 2022
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caalador caalador caalador approved these changes

Assignees
No one assigned
Labels
cherry-picked-7.0 +1.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[V20] Duplicate Lumo version when using npm
4 participants
@pleku @Artur- @vaadin-bot @caalador