feat: install an npm package only in the mode it declares - #25623
totally-not-ai[bot] wants to merge 3 commits into
Conversation
A versions file entry declaring `mode: lit` or `mode: react` says which mode installs the package. Only the pinning followed that mode though, while the package stayed a dependency of the application in the other mode, unless a versions file listed it in the `exclusions` array of the package installing it there. The mode now decides both: a package declared for a mode other than the one being built is excluded, the way the `exclusions` array did. This is what the javadoc of `getExclusions()` already describes, and what the `excludeWebComponents` build already did for the React packages. So the React components no longer have to enumerate every web component package they bring, and excluding web components no longer depends on a versions file listing them. A package that declares no mode is installed in every mode, as before, and the `exclusions` arrays keep working.
| // 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. | ||
| exclusions.add(npmName); |
There was a problem hiding this comment.
One issue here is that now we would drop aura as it is "mode": "lit" instead of "mode": "all".
Also any add-on that declares a mode that is not empty or all will be excluded.
There was a problem hiding this comment.
Both are real, thanks.
@vaadin/aura: 0c61cb8 makes a mode-based exclusion apply only where no versions file installs the package in the mode being built. The Aura theme module of flow-components 25.4 declares @vaadin/aura without a mode, so that entry keeps it installed with React and the mode: lit of the platform entry no longer drops it. The platform entry should still become all, so the platform file is right on its own: @vaadin/aura is not a dependency of @vaadin/react-components or @vaadin/react-components-pro, so nothing else brings it in. The same check says the other mode: lit packages that the React components do not exclude today — @vaadin/a11y-base, @vaadin/field-base, @vaadin/vaadin-lumo-styles and @vaadin/vaadin-themable-mixin — are dependencies of the React components, so they are installed either way.
The add-on case is not solved by that, and I would rather not pretend it is. mode cannot tell "installed in Lit mode only" from "pinned for Lit but needed in both", so an add-on declaring mode: lit for a package that nothing else provides loses it in a React build. The versions folder is public API since 25.3, so that is a behaviour change for add-ons that already ship such a file.
The alternative keeps the exclusions explicit and removes only the part that is maintained by hand: flow-components generates, for each of its modules, a @vaadin/react-components (or -pro) entry whose exclusions are that module's own packages. The union over the files on the classpath is the list the platform keeps today, it needs no change in Flow, and add-on files keep working unchanged. It costs one extra entry per module in the generated files.
Which would you rather have? I am happy to close this in favour of the generated exclusions.
There was a problem hiding this comment.
Is the solution to remove mode from aura and other common deps? Why do they have "mode: lit" today?
There was a problem hiding this comment.
They should have mode: all or no mode at all. Why they are set to lit I do not know.
There was a problem hiding this comment.
For @vaadin/aura yes, for the other common dependencies no — and the history answers the "why".
mode was added to versions.json in vaadin/platform#5076, and that commit states the invariant it was added under: it marks the core and pro web components for the Lit mode, and "All dependencies in lit mode are already included in react-components dependency". So mode: lit has meant "the React components bring this instead" from the start. This PR only makes it decide what is installed on top of what is pinned.
The Aura entry is the one that does not hold that invariant. It was added later, in vaadin/platform#8042 for V25, with the same shape as the component entries around it, and @vaadin/react-components does not depend on @vaadin/aura, so nothing brings the theme to a React application.
I checked every mode: lit entry against the dependency tree of the published @vaadin/react-components and @vaadin/react-components-pro: all of them are in it, including @vaadin/a11y-base, @vaadin/field-base, @vaadin/vaadin-lumo-styles, @vaadin/vaadin-themable-mixin, @vaadin/component-base and @vaadin/overlay. Those keep the mode. @vaadin/aura is the only entry that changes: vaadin/platform#9410 removes its mode.
Nothing changes in this pull request for it, the fix is the entry in versions.json.
There was a problem hiding this comment.
Agreed for @vaadin/aura, and it is now vaadin/platform#9410, which leaves the mode out of that entry.
It was set to lit because the entry was added for V25 in vaadin/platform#8042 with the same shape as the component entries next to it. mode itself came from vaadin/platform#5076, which added it under the invariant that every package marked for the Lit mode is already a dependency of @vaadin/react-components — and the Aura theme is not.
The other common dependencies do hold that invariant. @vaadin/a11y-base, @vaadin/field-base, @vaadin/vaadin-lumo-styles and @vaadin/vaadin-themable-mixin are all in the dependency tree of the published React components, so a React application gets them either way and mode: lit stays right for them.
| // 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. | ||
| exclusions.add(npmName); |
There was a problem hiding this comment.
mode-based exclusion leaks across versions files, contradicting the design this PR preserves for pinning
getExclusions() (PinnedNpmVersions.java:336-342) unions the per-file sets, so mode-based names become global. E.g. platform declares @vaadin/grid as mode: react, an add-on's file declares it unqualified, build is Lit: pinning still works, but ExclusionFilter drops grid from scanned deps, so the @NpmPackage version the add-on declared is silently discarded — defeating the add-on downgrade path guarded at TaskUpdatePackages.java:650. If nothing pins it in the active mode, it vanishes from package.json.
Mode-based exclusion should probably be file-scoped like the pinning is.
There was a problem hiding this comment.
Fixed in 0c61cb8 — you are right, and the rule was already spelled out for pinning right above it.
The converter now keeps the names it leaves out because of the mode in getModeExclusions(), separate from getExclusions(), which stays what the file excludes outright. PinnedNpmVersions.getExclusions() then removes from the mode set every package that any versions file installs in the mode being built, so the mode of one file no longer decides for the others.
Your @vaadin/grid case is now a test, packageLeftOutOfOneFileByItsMode_isNotExcludedWhenAnotherFileInstallsIt, next to the existing packageLeftOutOfOneFileByItsMode_isPinnedByTheFileDeclaringIt that covers the same rule for pinning. packageNoFileInstallsInTheMode_isExcluded covers the case that should still be excluded.
Leaving a package out because of the mode it is installed in was added to the exclusions of the versions file it came from, and getExclusions() unions those sets, so the mode of one file decided for all of them. A package another file installs in the mode being built was dropped from package.json even though its version was still pinned, which discards the version an add-on or an application declares for it. The mode exclusions are now kept apart from what a file excludes outright, and only the ones no file installs in this mode are excluded. This is the rule the pinned versions already follow: the mode of one file says nothing about what another one declares.
| @@ -177,14 +193,18 @@ private void addDependency(JsonNode obj) { | |||
| return; | |||
| } | |||
| if (!isIncludedByMode(mode)) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Only `lit` and `react` leave a package out of the other mode. Any other non-empty value was read as a mode of its own, which matched neither mode, so a typo took the package out of both of them and removed it from package.json altogether. Such a value says nothing about when the package is used, so the package is now installed in every mode, as one declaring no mode is, and the value is warned about.
|
|
To settle the question this thread raised: the mode is the right signal, and I withdraw the suggestion of closing this in favour of generated
On the two cases raised in review:
The four packages marked for the Lit mode that the React components do not exclude today — With the mode acting on it, the arrays are redundant: vaadin/platform#9413 removes both of them, 67 package names, and is marked as depending on this pull request so it does not land first. Every name they listed is a |
|
Let's get back to this, if it is still needed and in a more compatible way |
2 participants
Summary
A versions file entry can say
"mode": "lit"or"mode": "react". Until now that only decided which version was pinned — the package was still added to the application'spackage.jsonin the other mode. Now the mode decides both, so a package meant for one mode is no longer installed in the other.What changed
VersionsJsonConverternow excludes a package whose declared mode is not the mode being built, the same way anexclusionsarray does. This is what the javadoc ofgetExclusions()already described, and what theexcludeWebComponentsbuild already did for React packages.getModeExclusions(). They are not mixed into what a file excludes outright.PinnedNpmVersions.getExclusions()reads all versions files first and excludes a mode-based package only when no file installs it in the mode being built. One file's mode must not decide for another file — the same rule the pinned versions already follow. Without this, a package another file (an add-on or an application) installs and pins was dropped frompackage.jsonand its declared version was lost.Results: React components no longer have to list every web component package they bring, and excluding web components no longer depends on a versions file enumerating them. A package that declares no mode is installed in every mode, as before, and
exclusionsarrays keep working.Use case
An add-on ships a Lit-only web component plus a React wrapper that already bundles that web component. The author wants the plain web component installed only in Lit applications, and does not want to maintain an
exclusionslist in the React entry.They just declare the mode in their versions file:
{ "core": { "my-field": { "npmName": "@acme/my-field", "jsVersion": "2.1.0", "mode": "lit" } }, "react": { "my-field-react": { "npmName": "@acme/my-field-react", "jsVersion": "2.1.0", "mode": "react" } } }In a React application,
@acme/my-fieldis now left out ofpackage.jsonand@acme/my-field-reactis installed. In a Lit application it is the other way round. Noexclusionsarray needed.Test summary
getExclusions()orgetDeclaredExclusions()on a single filemode: litpackage is absent from bothdependenciesandvaadin.dependenciesofpackage.jsonpackage.jsonexcludeWebComponentsleaves web components out even when the versions file has noexclusionsarrayVersionsJsonConverterTest.modeExcludesThePackagesInstalledInTheOtherModeOnly— 1, 2VersionsJsonConverterTest.declaredExclusionsLeaveOutWhatTheModeExcludes— 2PinnedNpmVersionsTest.packageLeftOutOfOneFileByItsMode_isNotExcludedWhenAnotherFileInstallsIt— 3PinnedNpmVersionsTest.packageNoFileInstallsInTheMode_isExcluded— 4TaskUpdatePackagesNpmTest.reactEnabled_scannerDependencies_litOnlyDependenciesNotAdded— 5 (renamed and flipped, it previously asserted the old behaviour)TaskUpdatePackagesNpmTest.webComponentsExcluded_reactEnabled_noExclusionsInVersions— 6TaskUpdatePackagesNpmTest.webComponentsExcluded_reactDisabled_noExclusionsInVersions— 6Deliberately not covered by new tests: the existing
exclusionsarray behaviour, which is already pinned by unchanged tests in the same classes, and thegetModeExclusions()getter itself, which is only checked through the behaviour above.