Skip to content

feat: install an npm package only in the mode it declares #25623

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

Closed
totally-not-ai wants to merge 3 commits into from
Closed

feat: install an npm package only in the mode it declares #25623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ee7ff24
feat: install an npm package only in the mode it declares
totally-not-ai[bot] Sep 10, 2026
0c61cb8
fix: exclude a package by mode only where no file installs it
totally-not-ai[bot] Sep 10, 2026
637e66a
fix: install a package whose mode is not a mode in every mode
totally-not-ai[bot] Sep 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions flow-build-tools/src/main/java/com/vaadin/flow/server/frontend/PinnedNpmVersions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,8 @@ ObjectNode getDependencies(boolean reactEnabled,
}

/**
* Gets the npm packages that the versions files exclude.
* Gets the npm packages that the versions files exclude, including the ones
* no file installs in the mode being built.
*
* @param reactEnabled
* whether React is enabled
Expand All @@ -336,9 +337,22 @@ ObjectNode getDependencies(boolean reactEnabled,
Set<String> getExclusions(boolean reactEnabled,
boolean excludeWebComponents) {
Set<String> exclusions = new TreeSet<>();
files.forEach(file -> exclusions
.addAll(new VersionsJsonConverter(file.content(), reactEnabled,
excludeWebComponents).getExclusions()));
Set<String> modeExclusions = new TreeSet<>();
Set<String> installed = new TreeSet<>();
for (VersionsFile file : files) {
VersionsJsonConverter converter = new VersionsJsonConverter(
file.content(), reactEnabled, excludeWebComponents);
exclusions.addAll(converter.getExclusions());
modeExclusions.addAll(converter.getModeExclusions());
installed
.addAll(JacksonUtils.getKeys(converter.getConvertedJson()));
}
// A package left out of a file because of the mode it is installed in
// is only excluded where no file installs it in the mode being built:
// the mode of one file says nothing about what another one declares,
// the same way it does not for the versions being pinned
modeExclusions.removeAll(installed);
exclusions.addAll(modeExclusions);
return exclusions;
}

Expand Down
75 changes: 64 additions & 11 deletions flow-build-tools/src/main/java/com/vaadin/flow/server/frontend/VersionsJsonConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ class VersionsJsonConverter {

private Set<String> declaredExclusions;

private Set<String> modeExclusions;

private static Logger getLogger() {
return LoggerFactory.getLogger(VersionsJsonConverter.class);
}
Expand All @@ -91,6 +93,7 @@ private static Logger getLogger() {
this.excludeWebComponents = excludeWebComponents;
exclusions = new HashSet<>();
declaredExclusions = new HashSet<>();
modeExclusions = new HashSet<>();
convertedObject = JacksonUtils.createObjectNode();

collectDependencies(pinnedNpmVersions);
Expand All @@ -108,17 +111,30 @@ ObjectNode getConvertedJson() {
}

/**
* Get the exclusions set of npm package names.
* <p>
* Includes the packages left out because of the mode they apply to, which
* are excluded for this versions file rather than by the file saying so.
* Get the exclusions set of npm package names, the ones the versions file
* excludes wherever they are declared.
*
* @return the exclusions set
*/
Set<String> getExclusions() {
return exclusions;
}

/**
* Gets the npm package names left out because of the mode they are
* installed in, which are excluded for this versions file rather than by
* the file saying so.
* <p>
* A package another versions file installs in the mode being built is still
* installed, so these are only excluded where no file installs them, which
* the caller reading several files decides.
*
* @return the npm package names this file does not install in this mode
*/
Set<String> getModeExclusions() {
return modeExclusions;
}

/**
* Get the npm package names the versions file itself excludes, in its
* {@value #EXCLUSIONS} arrays.
Expand All @@ -145,14 +161,16 @@ private void collectDependencies(JsonNode obj) {

private void excludeDependencies() {
for (String key : JacksonUtils.getKeys(convertedObject)) {
if (exclusions.contains(key)) {
if (exclusions.contains(key) || modeExclusions.contains(key)) {
convertedObject.remove(key);
}
}
}

private boolean isIncludedByMode(String mode) {
if (mode == null || mode.isBlank() || MODE_ALL.equalsIgnoreCase(mode)) {
if (!isOneModeOnly(mode)) {
// A package that is not declared for one mode alone is installed
// in every mode, whether it says so or says nothing at all
return true;
} else if (excludeWebComponents) {
return false;
Expand All @@ -163,6 +181,36 @@ private boolean isIncludedByMode(String mode) {
}
}

/**
* Checks whether the mode is one that leaves the package out of the other
* mode, which only {@value #MODE_LIT} and {@value #MODE_REACT} are.
* <p>
* Anything else, a mode that is missing, empty, {@value #MODE_ALL} or a
* value that is not a mode at all, says nothing about when the package is
* used, so the package is installed in every mode. Reading an unknown value
* as a mode of its own would take the package out of both modes over a
* typo.
*/
private static boolean isOneModeOnly(String mode) {
return MODE_LIT.equalsIgnoreCase(mode)
|| MODE_REACT.equalsIgnoreCase(mode);
}

/**
* Warns about a mode that is not one of the modes there are, which is
* ignored so that the package is installed in every mode.
*/
private static void warnAboutUnknownMode(String npmName, String mode) {
if (mode == null || mode.isBlank() || MODE_ALL.equalsIgnoreCase(mode)
|| isOneModeOnly(mode)) {
return;
}
getLogger().warn(
"The npm package '{}' is declared for the mode '{}', which is not '{}', '{}' or '{}',"
+ " so it is installed in every mode. Report it to whoever ships the versions file.",
npmName, mode, MODE_LIT, MODE_REACT, MODE_ALL);
}

private void addDependency(JsonNode obj) {
assert obj.has(NPM_NAME);
String npmName = obj.get(NPM_NAME).asString();
Expand All @@ -176,15 +224,20 @@ private void addDependency(JsonNode obj) {
exclusions.add(npmName);
return;
}
warnAboutUnknownMode(npmName, mode);
if (!isIncludedByMode(mode)) {

@caalador caalador Sep 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

isIncludedByMode will always return false for non-blank values that are not all/lit/react, so any typo will remove a package in both modes.

@totally-not-ai totally-not-ai Bot Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, fixed in 637e66a.

Only lit and react leave a package out of the other mode now. isIncludedByMode returns true for anything else, so a value that is not a mode is treated the same as no mode at all and the package is installed in every mode instead of neither. The value is warned about, naming the package, so the typo is visible rather than silent.

This also covers a build that excludes web components: a package whose mode is not one of the modes is not identifiable as a web component package, so it is kept, as a package without a mode already was.

modeThatIsNotAMode_thePackageIsInstalledInEveryMode covers it for both modes, using "Lit " as the value.

// The package declares the mode it is installed in, and it is not
// the mode of this build, so this file does not install it here:
// whatever installs it in that mode brings it there instead, the
// way the React components bring the web components of a Lit
// package. Only a package declaring a mode gets here, as one
// without a mode is included in every mode.
// This says nothing about the other versions files though, so it
// is kept apart from what the file excludes outright.
modeExclusions.add(npmName);
if (excludeWebComponents) {
// collecting exclusions also from non-included dependencies
// with a mode (react), when web components are not wanted.
// The package is not installed from this file, so what it
// excludes is not something this file says about the package
if (MODE_REACT.equalsIgnoreCase(mode)) {
exclusions.add(npmName);
}
collectExclusions(obj, false);
}
return;
Expand Down
68 changes: 68 additions & 0 deletions flow-build-tools/src/test/java/com/vaadin/flow/server/frontend/PinnedNpmVersionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,74 @@ void packageLeftOutOfOneFileByItsMode_isPinnedByTheFileDeclaringIt()
.has("@vaadin/grid"));
}

@Test
void packageLeftOutOfOneFileByItsMode_isNotExcludedWhenAnotherFileInstallsIt()
throws IOException {
PinnedNpmVersions pinnedNpmVersions = createPinnedNpmVersions("""
{
"core": {
"grid": {
"npmName": "@vaadin/grid",
"jsVersion": "25.1.0",
"mode": "react"
}
}
}
""", """
{
"components": {
"grid": {
"npmName": "@vaadin/grid",
"jsVersion": "25.1.0"
}
}
}
""");

// The first file does not install the package without React, which
// says nothing about the file declaring it for every mode: excluding
// it would drop the version that file pins, and the one an add-on or
// an application declares for it
assertFalse(
pinnedNpmVersions.getExclusions(false, false)
.contains("@vaadin/grid"),
"A package another versions file installs should not be excluded");
}

@Test
void packageNoFileInstallsInTheMode_isExcluded() throws IOException {
PinnedNpmVersions pinnedNpmVersions = createPinnedNpmVersions("""
{
"core": {
"grid": {
"npmName": "@vaadin/grid",
"jsVersion": "25.1.0",
"mode": "lit"
}
}
}
""", """
{
"react": {
"react-components": {
"npmName": "@vaadin/react-components",
"jsVersion": "25.1.0",
"mode": "react"
}
}
}
""");

// Nothing installs the web component with React, so the React
// components bring it instead of the application installing it
assertTrue(pinnedNpmVersions.getExclusions(true, false)
.contains("@vaadin/grid"));
// And the other way round, the React package is not installed by a
// Lit application
assertTrue(pinnedNpmVersions.getExclusions(false, false)
.contains("@vaadin/react-components"));
}

@Test
void packageWithoutAVersion_theOtherPackagesArePinnedAllTheSame()
throws IOException {
Expand Down
20 changes: 13 additions & 7 deletions ...-build-tools/src/test/java/com/vaadin/flow/server/frontend/TaskUpdatePackagesNpmTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -816,7 +816,7 @@ void npmOverridesExist_customOverridesCopiedOver_verifyPnpmOverrides()
}

@Test
void reactEnabled_scannerDependencies_coreDependenciesNotAdded()
void reactEnabled_scannerDependencies_litOnlyDependenciesNotAdded()
throws IOException {
createVaadinVersionsJson(PINNED_DIALOG_VERSION,
PINNED_ELEMENT_MIXIN_VERSION, PINNED_OVERLAY_VERSION);
Expand All @@ -833,9 +833,11 @@ void reactEnabled_scannerDependencies_coreDependenciesNotAdded()
task.execute();
final ObjectNode newPackageJson = getOrCreatePackageJson();

assertTrue(newPackageJson.has("dependencies")
// The dialog is declared for the Lit mode, so the React components
// bring the web component instead of the application installing it
assertFalse(newPackageJson.has("dependencies")
&& newPackageJson.get("dependencies").has(VAADIN_DIALOG));
assertTrue(newPackageJson.has("vaadin") && newPackageJson.get("vaadin")
assertFalse(newPackageJson.has("vaadin") && newPackageJson.get("vaadin")
.get("dependencies").has(VAADIN_DIALOG));
assertTrue(newPackageJson.has("dependencies")
&& newPackageJson.get("dependencies").has(VAADIN_OVERLAY));
Expand Down Expand Up @@ -957,8 +959,10 @@ void webComponentsExcluded_reactDisabled_noExclusionsInVersions()
execTaskUpdatePackages(createApplicationDependencies(), options);
JsonNode pkgJson = getOrCreatePackageJson();

assertTrue(hasInDependencies(pkgJson, VAADIN_DIALOG));
assertTrue(hasInVaadinDependencies(pkgJson, VAADIN_DIALOG));
// The dialog declares a mode, so it is a web component package and
// is left out without the versions file having to list it
assertFalse(hasInDependencies(pkgJson, VAADIN_DIALOG));
assertFalse(hasInVaadinDependencies(pkgJson, VAADIN_DIALOG));
assertTrue(hasInDependencies(pkgJson, VAADIN_OVERLAY));
assertTrue(hasInVaadinDependencies(pkgJson, VAADIN_OVERLAY));
assertFalse(hasInDependencies(pkgJson, REACT_COMPONENTS));
Expand Down Expand Up @@ -1024,8 +1028,10 @@ void webComponentsExcluded_reactEnabled_noExclusionsInVersions()
execTaskUpdatePackages(createApplicationDependencies(), options);
JsonNode pkgJson = getOrCreatePackageJson();

assertTrue(hasInDependencies(pkgJson, VAADIN_DIALOG));
assertTrue(hasInVaadinDependencies(pkgJson, VAADIN_DIALOG));
// The dialog declares a mode, so it is a web component package and
// is left out without the versions file having to list it
assertFalse(hasInDependencies(pkgJson, VAADIN_DIALOG));
assertFalse(hasInVaadinDependencies(pkgJson, VAADIN_DIALOG));
assertTrue(hasInDependencies(pkgJson, VAADIN_OVERLAY));
assertTrue(hasInVaadinDependencies(pkgJson, VAADIN_OVERLAY));
assertFalse(hasInDependencies(pkgJson, REACT_COMPONENTS));
Expand Down
Loading
Loading