diff --git a/flow-build-tools/src/main/java/com/vaadin/flow/server/frontend/PinnedNpmVersions.java b/flow-build-tools/src/main/java/com/vaadin/flow/server/frontend/PinnedNpmVersions.java index 55b6dbafc23..ad1a3ad8e3a 100644 --- a/flow-build-tools/src/main/java/com/vaadin/flow/server/frontend/PinnedNpmVersions.java +++ b/flow-build-tools/src/main/java/com/vaadin/flow/server/frontend/PinnedNpmVersions.java @@ -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 @@ -336,9 +337,22 @@ ObjectNode getDependencies(boolean reactEnabled, Set getExclusions(boolean reactEnabled, boolean excludeWebComponents) { Set exclusions = new TreeSet<>(); - files.forEach(file -> exclusions - .addAll(new VersionsJsonConverter(file.content(), reactEnabled, - excludeWebComponents).getExclusions())); + Set modeExclusions = new TreeSet<>(); + Set 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; } diff --git a/flow-build-tools/src/main/java/com/vaadin/flow/server/frontend/VersionsJsonConverter.java b/flow-build-tools/src/main/java/com/vaadin/flow/server/frontend/VersionsJsonConverter.java index fbeb0020e2e..227fee8c211 100644 --- a/flow-build-tools/src/main/java/com/vaadin/flow/server/frontend/VersionsJsonConverter.java +++ b/flow-build-tools/src/main/java/com/vaadin/flow/server/frontend/VersionsJsonConverter.java @@ -81,6 +81,8 @@ class VersionsJsonConverter { private Set declaredExclusions; + private Set modeExclusions; + private static Logger getLogger() { return LoggerFactory.getLogger(VersionsJsonConverter.class); } @@ -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); @@ -108,10 +111,8 @@ ObjectNode getConvertedJson() { } /** - * Get the exclusions set of npm package names. - *

- * 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 */ @@ -119,6 +120,21 @@ Set 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. + *

+ * 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 getModeExclusions() { + return modeExclusions; + } + /** * Get the npm package names the versions file itself excludes, in its * {@value #EXCLUSIONS} arrays. @@ -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; @@ -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. + *

+ * 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(); @@ -176,15 +224,20 @@ private void addDependency(JsonNode obj) { exclusions.add(npmName); return; } + warnAboutUnknownMode(npmName, mode); if (!isIncludedByMode(mode)) { + // 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; diff --git a/flow-build-tools/src/test/java/com/vaadin/flow/server/frontend/PinnedNpmVersionsTest.java b/flow-build-tools/src/test/java/com/vaadin/flow/server/frontend/PinnedNpmVersionsTest.java index bcbad2608da..b7840c2790f 100644 --- a/flow-build-tools/src/test/java/com/vaadin/flow/server/frontend/PinnedNpmVersionsTest.java +++ b/flow-build-tools/src/test/java/com/vaadin/flow/server/frontend/PinnedNpmVersionsTest.java @@ -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 { diff --git a/flow-build-tools/src/test/java/com/vaadin/flow/server/frontend/TaskUpdatePackagesNpmTest.java b/flow-build-tools/src/test/java/com/vaadin/flow/server/frontend/TaskUpdatePackagesNpmTest.java index bbce8f05d18..fb59ebab85d 100644 --- a/flow-build-tools/src/test/java/com/vaadin/flow/server/frontend/TaskUpdatePackagesNpmTest.java +++ b/flow-build-tools/src/test/java/com/vaadin/flow/server/frontend/TaskUpdatePackagesNpmTest.java @@ -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); @@ -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)); @@ -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)); @@ -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)); diff --git a/flow-build-tools/src/test/java/com/vaadin/flow/server/frontend/VersionsJsonConverterTest.java b/flow-build-tools/src/test/java/com/vaadin/flow/server/frontend/VersionsJsonConverterTest.java index e8e597b54dc..f3774673af9 100644 --- a/flow-build-tools/src/test/java/com/vaadin/flow/server/frontend/VersionsJsonConverterTest.java +++ b/flow-build-tools/src/test/java/com/vaadin/flow/server/frontend/VersionsJsonConverterTest.java @@ -387,6 +387,94 @@ void testModeProperty() { assertFalse(convertedJson.has("react-components-pro")); } + @Test + void modeThatIsNotAMode_thePackageIsInstalledInEveryMode() { + String json = """ + { + "core": { + "text-field": { + "npmName": "@vaadin/text-field", + "jsVersion": "25.3.0", + "mode": "Lit " + } + } + } + """; + + // A value that is not a mode says nothing about when the package is + // used, so it is installed in every mode rather than in neither of + // them, which a typo would otherwise cause + for (boolean reactEnabled : new boolean[] { true, false }) { + VersionsJsonConverter convert = new VersionsJsonConverter( + JacksonUtils.readTree(json), reactEnabled, false); + assertTrue(convert.getConvertedJson().has("@vaadin/text-field"), + "The package should be pinned with react " + reactEnabled); + assertFalse( + convert.getModeExclusions().contains("@vaadin/text-field"), + "The package should be installed with react " + + reactEnabled); + assertFalse(convert.getExclusions().contains("@vaadin/text-field")); + } + } + + @Test + void modeExcludesThePackagesInstalledInTheOtherModeOnly() { + String json = """ + { + "core": { + "text-field": { + "npmName": "@vaadin/text-field", + "jsVersion": "25.3.0", + "mode": "lit" + }, + "date-fns": { + "npmName": "date-fns", + "jsVersion": "4.4.0" + } + }, + "react": { + "react-components": { + "npmName": "@vaadin/react-components", + "jsVersion": "25.3.0", + "mode": "react" + } + } + } + """; + + // With React, the Lit package is not installed, as the React + // components bring the web component instead + VersionsJsonConverter react = new VersionsJsonConverter( + JacksonUtils.readTree(json), true, false); + assertTrue(react.getModeExclusions().contains("@vaadin/text-field"), + "A Lit package should not be installed when React is used"); + assertFalse( + react.getModeExclusions().contains("@vaadin/react-components")); + assertFalse(react.getModeExclusions().contains("date-fns"), + "A package without a mode is installed in every mode"); + assertTrue(react.getConvertedJson().has("@vaadin/react-components")); + assertTrue(react.getConvertedJson().has("date-fns")); + + // Leaving a package out because of the mode is not something the file + // says about the package, so it does not exclude it from the others + assertFalse(react.getExclusions().contains("@vaadin/text-field")); + assertFalse( + react.getDeclaredExclusions().contains("@vaadin/text-field")); + + // Without React, it is the React package that is not installed + VersionsJsonConverter lit = new VersionsJsonConverter( + JacksonUtils.readTree(json), false, false); + assertTrue(lit.getModeExclusions().contains("@vaadin/react-components"), + "A React package should not be installed when Lit is used"); + assertFalse(lit.getModeExclusions().contains("@vaadin/text-field")); + assertFalse(lit.getModeExclusions().contains("date-fns")); + assertTrue(lit.getConvertedJson().has("@vaadin/text-field")); + assertTrue(lit.getConvertedJson().has("date-fns")); + assertFalse(lit.getExclusions().contains("@vaadin/react-components")); + assertFalse(lit.getDeclaredExclusions() + .contains("@vaadin/react-components")); + } + @Test void declaredExclusionsLeaveOutWhatTheModeExcludes() { String json = """ @@ -418,8 +506,11 @@ void declaredExclusionsLeaveOutWhatTheModeExcludes() { // The React package is left out because web components are excluded, // and the router because React is used, neither of which is something - // the file says about those packages - assertTrue( + // the file says about those packages. The React package is left out by + // its mode, so it is only excluded where no file installs it + assertTrue(convert.getModeExclusions() + .contains("@vaadin/react-components")); + assertFalse( convert.getExclusions().contains("@vaadin/react-components")); assertTrue(convert.getExclusions().contains("@vaadin/router")); assertFalse(convert.getDeclaredExclusions()