fix: ignore a configured Node version too old to run the frontend build - #25649
fix: ignore a configured Node version too old to run the frontend build#25649totally-not-ai[bot] wants to merge 5 commits into
Conversation
The node.version setting is also read from the token file, where an old Vaadin version may have written a Node version that today's Vite refuses to run on. Such a version was installed and used as configured, so the build failed with a version that is found nowhere in the project.
|
It is not clear from the description if this is a behavioral change or not. If the user has explicitly defined a node version to use through a system/Maven/Gradle property, then that should be used and a warning should be printed if the version is incompatible. In this case we should not auto install anything else I think. However, if the wrong version comes from some stale token file, we definitely should use the correct version. Which of the cases are actually changed here? |
Overriding a version that somebody has asked for on purpose is worse than a build that fails for a stated reason, so an explicitly configured Node version is only warned about. A version in the token file is not chosen for the project when the file was written by an older Vaadin version, so one that the frontend tooling cannot run on is ignored instead.
The floor for versions that Vaadin installs by itself is not what the frontend tooling needs, so a version between it and the supported minimum was warned about and dropped from the token file even though it runs. The warning also named the default version as the replacement for a configured node folder, where a global Node or a configured version is taken into use instead.
|
@Artur- Good point, and the first version of this did the wrong thing: it overrode any configured version. That is now split by where the version comes from. Unchanged — an explicitly configured version wins. A version from Changed — a The case this comes from is a project upgraded from Vaadin 24.0, where an IDE-launched dev mode still read One API addition: |
| NodeInstallation.forVersion(alternativeDirFile, nodeVersion)); | ||
| if (active != null) { | ||
| getLogger().debug("Node {} is already installed in {}", nodeVersion, | ||
| getLogger().info("Using Node {} from {}", active.nodeVersion(), |
There was a problem hiding this comment.
Let's not change this to info - most people don't want to see it on every startup
There was a problem hiding this comment.
Reverted to debug — the line only matters when the frontend build misbehaves, so it is not worth one in every startup.
| * | ||
| * @since 25.4 | ||
| */ | ||
| public static final FrontendVersion MINIMUM_SUPPORTED_NODE_VERSION = new FrontendVersion( |
There was a problem hiding this comment.
@Artur- Because the check runs in AbstractConfigurationFactory, which is in flow-server, while FrontendTools is in flow-build-tools — and that module depends on flow-server, not the other way around. Keeping the minimum in flow-server lets both use the same definition: FrontendTools.SUPPORTED_NODE_VERSION now points at it, so there is still one place where the number is written.
The alternative is to check the version in DevModeInitializer, which can see FrontendTools, but it no longer knows that the version came from the token file, so that needs a new flag written into the token file instead — the same amount of API, in a place where a system property can also set the version. Happy to change it if you prefer that split, or to drop the sharing and keep a separate constant in flow-server.
| FrontendUtils.MINIMUM_SUPPORTED_NODE_VERSION)) { | ||
| getLogger().warn( | ||
| "Ignoring Node.js version {} from '{}', as it is older than the minimum supported version {}. " | ||
| + "Run the 'prepare-frontend' goal to rewrite the file, or set the '{}' property to use that version anyway.", |
There was a problem hiding this comment.
prepare-frontend is deprecated and should not be suggested. Maybe the suggestion should be to clear the target folder instead?
There was a problem hiding this comment.
Changed to suggest clearing the build output folder, so that the file is written again: "The file is out of date, so clear the build output folder to have it written again, or set the 'vaadin.node.version' property to use that version anyway."
The line is of interest only when the frontend build misbehaves, so it is not worth a line in every startup. Also suggest clearing the build output folder instead of running a deprecated goal to get rid of an outdated Node version in the token file.
📦 Snapshot published
Built without running tests. Look at the checks on this pull request before relying on it. Every new commit on this branch republishes it while the How to use it<repository>
<id>vaadin-snapshots</id>
<url>https://maven.vaadin.com/vaadin-prereleases</url>
<snapshots><enabled>true</enabled></snapshots>
</repository>
<dependencyManagement>
<dependencies>
<dependency>
<groupId>com.vaadin</groupId>
<artifactId>flow-bom</artifactId>
<version>25.4.ignore-too-old-configured-node-version-SNAPSHOT</version>
<type>pom</type>
<scope>import</scope>
</dependency>
</dependencies>
</dependencyManagement>Import |
|
Is the potential real root cause here that an add-on packages a |
A token file inside a jar is only read when the project has none of its own, so it takes a second reading of the classpath to find a dependency that packages one by mistake. Say so in the warning, as clearing the build output folder does not help in that case.
|
@Artur- It can be, and the exclusion does apply here — there is only one token file read for every setting, so A jar copy is read when the project has none on the classpath: a packaged application, or a run started from a different output folder than the one the build wrote to. If several jars have one, the first is taken and "Unable to fully determine correct flow-build-info" is logged. Worth noting that a production build removes Either way the source does not change what happens now: the check does not care where the file came from, so a
|
|
1 participant
Summary
A Node.js version stored in the token file by an old Vaadin version could be installed and used, so the build failed on a version found nowhere in the project. Such a version is now ignored, and a version you configured yourself only gets a warning.
What changed
Behavior change: a
node.versionread from the token file (flow-build-info.json) that is older than the minimum supported Node.js version (24.0.0) is now dropped instead of used. Affected: only projects whose token file carries such an old version — typically a stale file in the build output folder, or a file packaged into a dependency by mistake. Those projects now fall back to the default Node.js version instead of failing the frontend build. A warning explains where the version came from, how to get rid of it, and how to keep it anyway (set thevaadin.node.versionproperty).Other changes:
NodeResolvernow warns when a Node.js version you configured yourself (throughnode.versionornode.folder) is older than the supported minimum. The configured version is still used — only a warning is logged, so nothing breaks for people who chose an old version on purpose.FrontendUtils.MINIMUM_SUPPORTED_NODE_VERSION.FrontendTools.SUPPORTED_NODE_VERSIONnow points at it, so the server and the build tools compare against the same floor. The minimum itself is unchanged (24.0); only its text form is now24.0.0.Apart from the token-file version being dropped, the change is backward compatible.
API Changes
com.vaadin.flow.internal.FrontendUtils
Test summary
node.versionolder than the supported minimum is not put into the configurationnode.versionis still passed throughlts) is passed through unchangednode.foldertriggers the warningDefaultApplicationConfigurationFactoryTest.create_tokenFileWithOutdatedNodeVersion_versionIsIgnored— rows 1, 2, 3NodeResolverTest.resolve_configuredVersionTooOld_isStillUsedAsConfigured— row 4Deliberately untested: the exact wording of the log messages, and the
node.folderwarning path, which needs a real Node installation folder to reach.