[DCA11Y-1145] Node version manager support #4

flipatlas · 2024-09-16T17:36:55Z

Changes:

add node version support for node and npm installers

...ugin/src/main/java/com/github/eirslett/maven/plugins/frontend/mojo/AbstractFrontendMojo.java

...gin/src/main/java/com/github/eirslett/maven/plugins/frontend/mojo/InstallNodeAndNpmMojo.java

...nd-plugin-core/src/main/java/com/github/eirslett/maven/plugins/frontend/lib/GlobalCache.java

...d-plugin-core/src/main/java/com/github/eirslett/maven/plugins/frontend/lib/NPMInstaller.java

.../java/com/github/eirslett/maven/plugins/frontend/lib/version/manager/VersionManagerType.java

atl-mk

A lot of work, but looking pretty good so far

atl-mk · 2024-09-17T09:05:41Z

I think I'd rather we get feature complete now so we can do one QA demo to rule them all.

I'm thinking in the QA demo we check;

version files not available and nothing specified in the POM (should give a good message instead of using the global version)

I can't think of anything else right now

atl-mk · 2024-09-17T09:06:24Z

Perhaps a good development loop is to set yourself up some docker containers, one for each version manager? Maybe you already having something working for you

atl-mk · 2024-09-17T09:11:20Z

One case I'm not sure about yet is no version file is specified and a version of Node is specified in the POM. I'm tempted to say we should do the vanilla behaviour and download it ourselves. We tell the users that the file is missing and they probably should create it.

This offers a simple way to drop-in this fork as a replacement in situations where they haven't previously created the files. That's because we'll have other features like incremental compilation which we want them to adopt.

- fix fnm when multiple node versions installed - use fnm dir to get installed node - disable tests on windows - fix paths in tests setup

flipatlas · 2024-09-19T10:59:53Z

I have skipped the new tests on windows, for now I'll assume we don't support windows. I'll revisit it later

…ties

… of version provided to fmp

…le found

flipatlas · 2024-09-20T14:12:24Z

QA Demo note: test on a project with split node installation in root and npm execution in modules

…ger-support # Conflicts: # FORK_CHANGELOG.md # frontend-maven-plugin/src/main/java/com/github/eirslett/maven/plugins/frontend/mojo/NodeAndNpmMojo.java # frontend-maven-plugin/src/main/java/com/github/eirslett/maven/plugins/frontend/mojo/NpmMojo.java

+ update prebuild script output streams + fix npmmojo after nodemojo revert + fix fnm node directory

atl-mk · 2024-12-06T15:56:28Z

...ugin/src/main/java/com/github/eirslett/maven/plugins/frontend/mojo/AbstractFrontendMojo.java

+     * Enables node installation using node version manager
+     */
+    @Parameter(property = "useNodeVersionManager", defaultValue = "true")
+    private boolean useNodeVersionManager;


Can we do one more like you suggested before? Just direct path to where node binary is. Both Maven property and use System.getEnv. I'm worried that someone will use an old branch with old version of this in the future and it won't support new version of FNM/NVS/etc. If we can tell them "no worries" modify your .mvn/cnfig and/or shell envinroment, that will be a great get out of jail pass.

Added installedNodeDirectory property and AFMP_INSTALLED_NODE_DIRECTORY env var for specifying node explicitly

...gin/src/main/java/com/github/eirslett/maven/plugins/frontend/mojo/InstallNodeAndNpmMojo.java

atl-mk · 2024-12-06T16:01:59Z

...tend-maven-plugin/src/main/java/com/github/eirslett/maven/plugins/frontend/mojo/NpmMojo.java

@@ -84,6 +102,19 @@ protected boolean skipExecution() {

    @Override
    public synchronized void execute(FrontendPluginFactory factory) throws Exception {
+        String nodeVersion = getNodeVersion(workingDirectory, this.nodeVersion, this.nodeVersionFile, project.getArtifactId(), getFrontendMavenPluginVersion());


We'll need to update metrics too so we can say x% of time using from version manager

atl-mk · 2024-12-06T16:04:40Z

...tend-maven-plugin/src/main/java/com/github/eirslett/maven/plugins/frontend/mojo/NpmMojo.java

+        }
+
+        String validNodeVersion = getDownloadableVersion(nodeVersion);
+        factory.loadNodeVersionManager(validNodeVersion);


Note to myself: Does this actually install if it's missing? If it does, not sure we want that? Probably don't want to encourage this, should be installing once in the parent POM.

It doesn't, it only sets the values of node and npm executable paths if version manager is available

atl-mk · 2024-12-06T16:06:00Z

...-maven-plugin/src/main/java/com/github/eirslett/maven/plugins/frontend/mojo/WebpackMojo.java

+        }
+
+        String validNodeVersion = getDownloadableVersion(nodeVersion);
+        factory.loadNodeVersionManager(validNodeVersion);


How did it get the webpack binary before? Presumably out of node_modules/.bin? How did it know the node version, relied on it being in the FMP managed node installation folder before?

...core/src/main/java/com/github/eirslett/maven/plugins/frontend/lib/FrontendPluginFactory.java

FORK_CHANGELOG.md

frontend-maven-plugin/src/it/node-version-manager/asdf/prebuild.groovy

atl-mk · 2024-12-06T16:53:50Z

...in/java/com/github/eirslett/maven/plugins/frontend/lib/version/manager/client/FnmClient.java

+        Platform platform = installConfig.getPlatform();
+        String architecture = platform.getArchitectureName();
+        String osCodename = platform.getCodename();
+        String nodeOnPlatform = String.format("node-%s-%s-%s", nodeVersionWithV, osCodename, architecture);


All my local installastions are like:
fnm/node-versions/v6.17.1/installation/bin. maybe only macOS problem?

until we can better reproduce, not sure the oscodename and architecture will perfectly match, hopefully they do and they might both be based off NodeJS' zip filename scheme

...ava/com/github/eirslett/maven/plugins/frontend/lib/version/manager/VersionManagerRunner.java

atl-mk · 2024-12-06T17:02:35Z

...va/com/github/eirslett/maven/plugins/frontend/lib/version/manager/VersionManagerLocator.java

+        VersionManagerFactory versionManagerFactory = new VersionManagerFactory(installConfig);
+        for (VersionManagerType versionManagerType : VersionManagerType.values()) {
+            VersionManagerClient versionManagerClient = versionManagerFactory.getClient(versionManagerType);
+            if (versionManagerClient.isInstalled()) return versionManagerType;


I like trying each version manager so people can have multiple installed without issues, e.g. fnm for everything except mise for JSM. Problem is we should adjust the logs to be less noisy. Single output

ATM it only checks if version manager directory is available but it doesn't check if node version is available in the version manager. This could be improved to choose version manager based on node versions available

atl-mk · 2024-12-06T17:03:36Z

...java/com/github/eirslett/maven/plugins/frontend/lib/version/manager/VersionManagerCache.java

+
+    }
+
+    public VersionManagerType getVersionManagerType() {


super nit: don't really gain anything from getters/setters when this isn't API. IDEs ca refeactor this almost instantly.

Yup, it was autogenerated by IDE in the first place. Some of the getters have extra logic so using getters for everything makes it uniform

atl-mk · 2024-12-06T17:05:56Z

.../src/main/java/com/github/eirslett/maven/plugins/frontend/lib/ProcessExecutionException.java

can probably avoid extraction if it's nested classclass, should be able to make public static nested. then it's only two line change

The exception is used in all the executor classes (BunExecutor, YarnExecutor, etc..). Nesting it will require changes in those classes instead AFAICS

...-plugin-core/src/main/java/com/github/eirslett/maven/plugins/frontend/lib/NodeInstaller.java

atl-mk · 2024-12-06T17:11:32Z

...nd-plugin-core/src/main/java/com/github/eirslett/maven/plugins/frontend/lib/GlobalCache.java

Is this cache shared between <executions> (the Maven definiton)? If so, will cause problems if configuration is changing (it can) between them?

atl-mk · 2024-12-06T17:12:30Z

Sorry, I'm not trying to sound pass-agro, I reviewed, but I don't know the ansers yet till I read the code and play with it more.

flipatlas added 2 commits September 16, 2024 11:32

DCA11Y-1145: add tests for nvm and fnm

d2f488e

DCA11Y-1145: add node version manager support to node and npm installers

0a771db

flipatlas requested a review from atl-mk September 16, 2024 17:36

DCA11Y-1145: fix setup for npm goal

a883c9c

atl-mk reviewed Sep 17, 2024

View reviewed changes

...ugin/src/main/java/com/github/eirslett/maven/plugins/frontend/mojo/AbstractFrontendMojo.java Outdated Show resolved Hide resolved

atl-mk reviewed Sep 17, 2024

View reviewed changes

...gin/src/main/java/com/github/eirslett/maven/plugins/frontend/mojo/InstallNodeAndNpmMojo.java Outdated Show resolved Hide resolved

atl-mk reviewed Sep 17, 2024

View reviewed changes

...nd-plugin-core/src/main/java/com/github/eirslett/maven/plugins/frontend/lib/GlobalCache.java Outdated Show resolved Hide resolved

atl-mk reviewed Sep 17, 2024

View reviewed changes

...d-plugin-core/src/main/java/com/github/eirslett/maven/plugins/frontend/lib/NPMInstaller.java Outdated Show resolved Hide resolved

atl-mk reviewed Sep 17, 2024

View reviewed changes

.../java/com/github/eirslett/maven/plugins/frontend/lib/version/manager/VersionManagerType.java Outdated Show resolved Hide resolved

atl-mk approved these changes Sep 17, 2024

View reviewed changes

flipatlas and others added 2 commits September 19, 2024 08:41

DCA11Y-1145: small CR tweaks

46b8a10

Issue/master/dca11 y 1145 node version manager support 2 (#5)

348cd57

- fix fnm when multiple node versions installed - use fnm dir to get installed node - disable tests on windows - fix paths in tests setup

flipatlas and others added 13 commits September 19, 2024 15:20

DCA11Y-1145: add test for yarn with fnm

c772fad

DCA11Y-1145: enable node version manager support by default

4180868

DCA11Y-1145: add NVS support

b04f793

DCA11Y-1145: fix bun default setup

bea814f

DCA11Y-1145: try with bash

5b8e67b

DCA11Y-1145: run prebuild installation with bash

8ba94c9

DCA11Y-1145: mise support

7d36ade

DCA11Y-1145: asdf support

111d2f6

restructure invoker tests to be hierarchical and share invoker.proper…

d946a7a

…ties

DCA11Y-1145: use node version manager configured node version instead…

8a6d23c

… of version provided to fmp

DCA11Y-1145: fix java docs

cbf1733

DCA11Y-1145: fail quickly when nvm can't fails, e.g. no versioning fi…

0e2c515

…le found

DCA11Y-1145: verify node version before installation

f4a7b64

flipatlas added 10 commits November 25, 2024 15:25

chore: clean up after release

eabb449

clean up documentation

6f623e1

install custom npm in nvm

1f0a3e7

fix custom node assertion

0abce14

try fix mise

5a27505

revert AbstractNodeMojo.java

7b03cc8

update mise client

70d139c

+ update prebuild script output streams + fix npmmojo after nodemojo revert + fix fnm node directory

chore: prepare for release 1.15.1-atlassian-4-70d139cc

b9364f6

chore: clean up after release

0b29e57