Open bmeck opened 6 years ago
Thanks for starting this issue @bmeck! We've done some thinking on this for Polymer as well, probably best summarized by @rictic here: https://github.com/Polymer/polymer/issues/4823#issuecomment-326772504
tl;dr: The current "flat" behavior of Yarn/Bower is really the conflation of two package qualities: "is web loadable" & "requires uniqueness".
Fleshing out how to signal the more general "unique"/"singleton" here could be helpful for making web projects (and hopefully web components!) on npm easier to work with.
Instead of defining (& maintaining) a brand new "singletonDependencies" dependencies group, could we leverage the "resolutions"
mechanism already used by both Bower & Yarn? This could work across your main dependencies, dev, etc.
Documented here: https://yarnpkg.com/lang/en/docs/cli/install/#toc-yarn-install-flat
Update: After re-reading this comment from @zkat it sounds like support for "resolutions" is already planned for npm as well: https://github.com/Polymer/polymer/issues/4823#issuecomment-329939498
@FredKSchott "resolutions"
does not search the entire tree for nested packages to my knowledge in Yarn.
/app
/node_modules/a
/node_modules/c # version 2.0.0
/node_modules/b
/node_modules/c # version 2.0.0
My personal usage is based upon non-flat installs using webpack. If we could mandate that the "resolutions"
field be enforced across nested dependencies that seems sane. However, I am not sure this version pinning is needed when we have the version range already in package.json
and resolved versions in the different lockfile/shrinkwrap files.
$ yarn list supports-color
├─┬ mocha@3.5.3
│ └── supports-color@3.1.2
└─┬ chalk@1.1.3
└── supports-color@2.0.0
"resolutions": {"supports-color": "2.0.0"}
in package.json
:$ yarn list supports-color
warning Resolution field "supports-color@2.0.0" is incompatible with requested version "supports-color@3.1.2"
└─ supports-color@2.0.0
"resolutions": {"supports-color": "^2.0.0"}
:$ yarn list supports-color
└─ supports-color@2.0.0
So it dedupes and hoists to the top level (but isn't perfect, some unclear warnings appear depending on whether it resolves to a semver range or specific version).
Right now it seems unreliable:
resolutions@1.0.0 /Users/bfarias/Documents/tmp/resolutions
├─┬ a@1.0.1
│ └── c@1.0.1
└─┬ b@1.0.1
└── c@1.0.1
LMDV-BRADLEY:resolutions bfarias$ yarn list c
yarn list v0.21.3
✨ Done in 1.25s.
LMDV-BRADLEY:resolutions bfarias$ cat package.json
{
"name": "resolutions",
"version": "1.0.0",
"main": "index.js",
"license": "MIT",
"resolutions": {
"c": "1.0.0"
},
"bundledledDependencies": ["a", "b"],
"dependencies": {
"a": "*",
"b": "*"
}
}
We should figure out what exactly it is doing today before using it.
@bmeck your lockfile may be getting in the way. If it keeps causing problems consider filing an issue with Yarn since top-level, deduped resolution was the goal for --flat
& resolutions
(source: I helped with the original implementation).
@FredKSchott I've tried several different approaches of installing and none seem to complain about nested dependencies, nor duplicates of the same version.
@FredKSchott https://github.com/yarnpkg/yarn/issues/4585 seems to be the issue I think?
However, it still doesn't enforce that only a single copy of that name/version combination exists in the nested dep graph.
@bmeck both yarn/npm hoist packages to the top level (& dedup) when no version conflicts exist. By resolving all occurrences to a single version, it logically follows that that each occurrence of that dep will be hoisted & deduped to a single, top-level dependency.
@FredKSchott things like bundledDependencies
can prevent that hoisting without version conflicts.
@bmeck yarn doesn't support bundledDependencies
, AFAIK
@iarna it at least has documentation for it: https://yarnpkg.com/lang/en/docs/dependency-types/#toc-bundleddependencies
I wonder if that’s just for publication… It used to be that it just ignored the bundles, but maybe they finally implemented that. It’s tricky for them because bundles define a specific version and are not guaranteed to be equivalent to the version on the registry, and so can’t be put in a place where other modules can load them. (That is, flattening them is definitionally invalid.) The tricky bit is that their lock file has no way AFAIK to indicate this. It’s part of why npm has stuck with a lockfile that indicates tree shape.
Edit: When I mentioned this, they quickly patched in something like bundled dep support, but I wouldn't use it just yet, it's still half baked. Currently you end up with multiple copies of all of your dependencies, using bundled deps w/ yarn.
Hey all, especially @bmeck and @iarna,
I wanted to bump this issue with some added interest and support from the Polymer team. I've talked independently to Bradley about this, but as we hone our strategy for moving to npm, supporting existing projects that can't use flat installations, but could use some singleton packages seems more and more important. This would be great for polyfills and other classes of libraries too.
I think this may be obsoleted (at least for the Polymer team's use case) by assetDependencies
.
@iarna my use case is not purely for front end that has a compilation step on installation, so mine remains.
@bmeck I don't understand how a compilation step is relevant to the discussion here? (That seems like a total non-sequitor.)
@iarna thats what assetDependencies
does when installing things as I understand it, and it is only focused on front end. We have front end compilation that is not suitable for assetDependencies
in production at GoDaddy right now.
@bmeck Ah, well, that's neither here nor there. Build steps on top of assetDependencies would be common, I would imagine. Nontheless, I wasn't commenting on your full proposal, just on the portion we had been in discussion w/ Polymer about.
@iarna it's not obsoleted for us mainly because we know we have customers, and potential customers, with existing projects that want to incrementally adopt packages that need more aggressive deduplication, like polyfills and custom elements. As nice as assets
, especially for new projects, we want to help out the existing ones too.
@justinfagnani it might be really helpful at this point to speak about the problems these customers and potential customers are having, how they're trying to solve them now, show example of why those solutions don't work, and show examples of how assets
specifically would not work for them.
I'm not convinced this proposal, as stated, solves the problem as much as it creates other, new problems to deal with.
@zkat can you describe the problems you see being created by this singleton check?
@bmeck I asked first.
@zkat ?
@bmeck I want to hear why and how it solves complete problems first. My experience with the last feature we had that had similar behavior, peerDependencies
, was very much not a positive one. I want to hear why current solutions don't work, why assets
would not work for solving those problems, and how this proposal compensates for the problems with the former manifestation of peerDependencies
.
Ah, I took your comment as very derisive versus that explanation which is much less hostile.
@bmeck I'd like to focus on keeping the conversation on a single topic, and on rails, as well as based on concrete problems, real-world examples, and multiple possible solutions focused on those problems.
@zkat in that case, I can provide a simple example from work.
We have had situations where react-intl
and some polyfills have been loaded with specified versions that cause duplication in the module graph. This behavior has been undesirable for those packages, but we do have other packages that correctly are duplicates such as when we do staged transitions between react
versions and have multiple versions on a single page. Having support for discrete multiple versions of react
is great in this situation, but being able to ensure that react-intl
does not get accidentally duplicated also is great.
[edit/addendum]: This situation has been a minor source of trouble in production in the past with real affects for localization. We don't necessarily control all dependencies per page since other teams do manage their applications outside of my team's control.
@bmeck in this specific example: if singleton: true
were declared by React
(which expects to be a singleton, so it's reasonable for them to use this flag), then you would not even be able to attempt loading multiple versions the way you are.
This specific example seems pretty corner-casey to me, unless we're talking about the separate feature of allowing multiple versions of a same-name package to live side-by-side, possibly in the same tree layer. Perhaps a different approach, or even manual manipulation of package-lock.json
would serve better in this case? That allows you to freely manipulate the tree and gives a good escape hatch when these issues crop up.
@zkat I'm not sure React
needs to be treated as a singleton since it doesn't use a global store to persist data in an externally mutable/observable way. react-intl
does however keep a singleton based store due to the addLocaleData API
@bmeck https://reactjs.org/warnings/refs-must-have-owner.html it does, indeed, require a single, non-conflicting version to be loaded, last I checked.
@zkat if you pass Components created from differing versions of React between the opposing version you do see those errors, yes. If you just have multiple React versions that do not pass components to the opposing version it has worked fine for us so far (fingers crossed it keeps working).
@bmeck this definitely sounds to me like, in this concrete example, you're describing adding an extra layer of complexity to a problem that you're already "getting away with" while crossing your fingers, rather than providing a solid foundation of a solution to a much more common, larger problem.
@zkat Our needs are complex yes. The finger crossing is more that React doesn't prevent transitions using multiple versions by forcing there to be a single version. However, I also am crossing my fingers for being able to get problematic packages that do not support multiple versions to mark themselves as such. React as it is today doesn't cause problems with multiple versions to my usage.
I'm not sure I would state my alternatives as solid foundations for now, but I will be happy if I can have both use cases above. Removing one of my use cases doesn't seem a way forward for progressive migration strategies that use multiple versions; and manually editing package-lock.json
seems error prone and still removes the capability to do progressive migration while 2 versions of a package are in use unless you rename the packages to be something like react
and react-16
/react-next
.
To me, this sounds like what peer dependencies already are. I wonder if perhaps a cleaner solution is a top-level package.json key like "requiredSingletons": ["react"]
, and then npm can check that in the entire graph for you?
Another companion change could be, like the (thankfully dead) preferGlobal, preferPeer
- that way packages can declare themselves as one that prefers to be a peer dependency.
I think re-using peerDependencies would fulfill my use cases if the desire is to avoid introducing a new type of dependency. A way to error on failed preferPeer
would be nice, but that should be easy to grab and tool off logs output by npm
.
Ping on this issue 😄
On Polymer we've moved to publishing our modules using package names in import specifiers, anticipating movement on support in browsers via something like https://github.com/domenic/package-name-maps
We still have requirements that certain packages are singletons, ie polyfills, frameworks and custom elements. If npm supported this it would help our users considerably. Right now we're still recommending yarn --flat
if a project can use it, because flatness guarantees uniqueness.
Some packages are intended to only exist within a dependency graph once. often things expecting to act like singletons.
I think it would be good to investigate 2 possible fields for
package.json
tooling to use."singleton": boolean
If present, the package must have a name unique within the entire module graph, else it will fail to install. This would prevent things like having 2 different versions of
react-intl
be able to install in the same graph."singletonDependencies": string[] | "all"
If present this field ensures that dependencies with the same name are not present multiple times within a graph. This could be useful for preventing things like having multiple configuration packages which might not wish to use
"singleton"
in their ownpackage.json
.If the value is an array of strings, it does search of the graph to ensure that the name is a singleton. This applies to the entire graph, not only the submodules of the current package.
If the value is "all" then all submodules must be singletons and their submodules recursively must also be singletons.
determination of "package name"
Module names are not found within
package.json
files. They are determined by the presence of a file with a filename that could be loaded as a the starting segments of a bare specifier likerequest
forimport("request")
orlodash
forimport("lodash/chunk")
.scoped packages
Installation of packages by tools are given the full name by which the package is to be imported:
If the package contained
"singleton": true
. This would result in singleton checks against all the dependency graph for@npm/invalid
before placing the installation such that it could be used viaimport("@npm/invalid")
. It would not check the value of"name"
within thepackage.json
that is installed.bundledDependencies
Bundled dependencies are also checked to be singletons, no exceptions are to be made when searching for collisions. If you wish to use a duplicate package in a bundled fashion: you can rename it, add a scope, or put it into a vendor folder.
root package
When writing applications, they tend to be at the root/entry point of your dependency graph. The directory name of this entry point and
package.json
"name"
field are not used when searching for singletons. This allows modules to link to themselves in such a way that they can be loaded as bare specifiers while still being declared singletons.realpathing
Due to the nature of various runtime flags like
--preserve-symlinks
, realpathing should not be performed when searching for singletons.