feat: lock versions with npm overrides

[haijian-vaadin]

Description

How it works?

1. generate versions.json also for npm mode
2. if npm mode, create an "overrides" section in the package.json file if the file does not exist.
3. iterate through dependencies in the version.json file, add each dependency to the overrides section as @vaadin/xxx: $@vaadin/xxx if there is no @vaadin/xxx in the "overrides" section.

Requires npm 8.3.0 or newer which has the "overrids" feature.
Fixes vaadin/hilla#164

Type of change

• Bugfix
• Feature

Checklist

• I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
• I have added a description following the guideline.
• The issue is created in the corresponding repository and I have referenced it.
• I have added tests to ensure my change is effective and works as intended.
• New and existing tests are passing locally with my change.
• I have performed self-review and corrected misspellings.

Additional for Feature type of change

• Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

[github-actions]

Unit Test Results

   769 files  ±0     769 suites  ±0   26m 46s ⏱️ + 3m 38s
5 742 tests +2  5 690 ✔️ +2  52 💤 ±0  0 ❌ ±0 
5 780 runs  +8  5 727 ✔️ +8  53 💤 ±0  0 ❌ ±0 

Results for commit 9660ba3. ± Comparison against base commit 9a5dbe9.

♻️ This comment has been updated with latest results.

[vaadin-bot]

SonarQube analysis reported 7 issues

1. TaskRunNpmInstall.java#L25: Remove this unused import 'java.nio.file.Path'.
2. TaskRunNpmInstall.java#L238: Refactor this method to reduce its Cognitive Complexity from 17 to the 15 allowed.
3. TaskUpdatePackages.java#L87: Constructor has 9 parameters, which is greater than 7 authorized.
4. TaskUpdatePackages.java#L105: Correct this "|" to "||".
5. TaskUpdatePackages.java#L198: Refactor this method to reduce its Cognitive Complexity from 20 to the 15 allowed.
6. TaskUpdatePackages.java#L250: Take the required action to fix the issue indicated by this comment.
7. TaskRunNpmInstall.java#L476: Remove the declaration of thrown exception 'java.lang.IllegalStateException' which is a runtime exception.

[joheriks]

Requires npm 8.3.0 or newer which has the "overrids" feature.

Should the version validation be updated then?

flow/flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendTools.java

Lines 113 to 114 in 5e19cb3

	private static final int SUPPORTED_NPM_MAJOR_VERSION = 6;
	private static final int SUPPORTED_NPM_MINOR_VERSION = 14;

[haijian-vaadin]

Requires npm 8.3.0 or newer which has the "overrids" feature.

Should the version validation be updated then?

flow/flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendTools.java

Lines 113 to 114 in 5e19cb3

	private static final int SUPPORTED_NPM_MAJOR_VERSION = 6;
	private static final int SUPPORTED_NPM_MINOR_VERSION = 14;

@Artur- did a change to upgrade to Node 17 + npm 8.3. Is that enough?
I think npm 6.14 is still supported, as in V14, just that it doesn't have the overrides feature available.

[Artur-]

It would be nice to let the users know that it will work better if you upgrade to npm 8.3+ if they are using an older globally installed node/npm. We cannot really require the latest version of npm though and we need to make sure that node 16 with its bundled npm works in a standard (i.e no addons) project

[joheriks]

So if I have an older npm version that doesn't support overrides the section will be ignored, and there will be no version locking (as it is currently working when using npm instead of pnpm). Is there a risk of a silent regression if I have been relying on pnpm now that the default switched to npm but my pre-installed version is too old?

[haijian-vaadin]

Is it enough to mention this in the upgrade guide? For people relying on the pnpm locking and cannot upgrade to npm 8.3, they can keep using pnpm. Could also make it a warning message in the code.

[Artur-]

The place you would run into this is most of the time when you get a browser warning that a custom element has already been defined. This would be the best place to provide additional info on how the solve the problem but it feels like a separate issue/pr

[caalador]

Also missing is cleaning of the override in CleanFrontendMojo

[haijian-vaadin]

Also missing is cleaning of the override in CleanFrontendMojo

Good catch, done

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

[imsys]

now that it will work better if you upgrade to npm 8.3+ if they are using an older globally installed node/npm. We cannot really require the latest version of npm though and we need to make sure that node 16 with its bundled npm works in a standard (i.e no addons) project

As someone who fell in this thread almost by accident, when I saw the issue at pnpm/pnpm#4190, and I don't know much about this project, but I thought to give my two cents, that you could add some version specification in the package.json

"engines": {
    "npm": ">=8.3.0",
    "pnpm": ">=5.10.1"
}

"Unless the user has set the engine-strict config flag (see .npmrc), this field is advisory only and will only produce warnings when your package is installed as a dependency." - from npmjs documentation

[haijian-vaadin]

Hi @imsys, thanks a lot for the proposal, that's a very good idea, I made an enhancement ticket out of your suggestion.