Skip to content
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.

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

javascript: singleton packages #3

Open
bmeck opened this issue Sep 21, 2017 · 38 comments
Open

javascript: singleton packages #3

bmeck opened this issue Sep 21, 2017 · 38 comments

Comments

@bmeck
Copy link

bmeck commented Sep 21, 2017

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 own package.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 like request for import("request") or lodash for import("lodash/chunk").

scoped packages

Installation of packages by tools are given the full name by which the package is to be imported:

npm i @npm/invalid

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 via import("@npm/invalid"). It would not check the value of "name" within the package.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.

@bmeck bmeck changed the title standalone packages singleton packages Sep 23, 2017
@FredKSchott
Copy link

Thanks for starting this issue @bmeck! We've done some thinking on this for Polymer as well, probably best summarized by @rictic here: Polymer/polymer#4823 (comment)
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.

@FredKSchott
Copy link

FredKSchott commented Oct 2, 2017

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: Polymer/polymer#4823 (comment)

@bmeck
Copy link
Author

bmeck commented Oct 2, 2017

@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.

@FredKSchott
Copy link

FredKSchott commented Oct 2, 2017

Without resolution:
$ yarn list supports-color
├─┬ [email protected]
│ └── [email protected] 
└─┬ [email protected]
  └── [email protected] 
With "resolutions": {"supports-color": "2.0.0"} in package.json:
$ yarn list supports-color
warning Resolution field "[email protected]" is incompatible with requested version "[email protected]"
└─ [email protected]
Funny enough, with "resolutions": {"supports-color": "^2.0.0"}:
$ yarn list supports-color
└─ [email protected]

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).

@bmeck
Copy link
Author

bmeck commented Oct 2, 2017

Right now it seems unreliable:

[email protected] /Users/bfarias/Documents/tmp/resolutions
├─┬ [email protected]
│ └── [email protected]
└─┬ [email protected]
  └── [email protected]

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.

@FredKSchott
Copy link

@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).

@bmeck
Copy link
Author

bmeck commented Oct 2, 2017

@FredKSchott I've tried several different approaches of installing and none seem to complain about nested dependencies, nor duplicates of the same version.

@FredKSchott
Copy link

🙏 ➡️ https://github.com/yarnpkg/yarn/issues/new

@bmeck
Copy link
Author

bmeck commented Oct 3, 2017

@FredKSchott yarnpkg/yarn#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.

@FredKSchott
Copy link

@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.

@bmeck
Copy link
Author

bmeck commented Oct 3, 2017

@FredKSchott things like bundledDependencies can prevent that hoisting without version conflicts.

@iarna iarna changed the title singleton packages javascript: singleton packages Oct 26, 2017
@iarna
Copy link
Member

iarna commented Oct 26, 2017

@bmeck yarn doesn't support bundledDependencies, AFAIK

@bmeck
Copy link
Author

bmeck commented Oct 30, 2017

@iarna it at least has documentation for it: https://yarnpkg.com/lang/en/docs/dependency-types/#toc-bundleddependencies

@iarna
Copy link
Member

iarna commented Nov 1, 2017 via email

@justinfagnani
Copy link

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.

@iarna
Copy link
Member

iarna commented Dec 6, 2017

I think this may be obsoleted (at least for the Polymer team's use case) by assetDependencies.

@bmeck
Copy link
Author

bmeck commented Dec 6, 2017

@iarna my use case is not purely for front end that has a compilation step on installation, so mine remains.

@iarna
Copy link
Member

iarna commented Dec 6, 2017

@bmeck I don't understand how a compilation step is relevant to the discussion here? (That seems like a total non-sequitor.)

@bmeck
Copy link
Author

bmeck commented Dec 6, 2017

@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.

@iarna
Copy link
Member

iarna commented Dec 6, 2017

@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.

@justinfagnani
Copy link

@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.

@zkat
Copy link
Member

zkat commented Dec 6, 2017

@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.

@bmeck
Copy link
Author

bmeck commented Dec 6, 2017

@zkat can you describe the problems you see being created by this singleton check?

@zkat
Copy link
Member

zkat commented Dec 6, 2017

@bmeck I asked first.

@bmeck
Copy link
Author

bmeck commented Dec 6, 2017

@zkat ?

@zkat
Copy link
Member

zkat commented Dec 6, 2017

@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.

@bmeck
Copy link
Author

bmeck commented Dec 6, 2017

Ah, I took your comment as very derisive versus that explanation which is much less hostile.

@zkat
Copy link
Member

zkat commented Dec 6, 2017

@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.

@bmeck
Copy link
Author

bmeck commented Dec 6, 2017

@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.

@zkat
Copy link
Member

zkat commented Dec 6, 2017

@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.

@bmeck
Copy link
Author

bmeck commented Dec 6, 2017

@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

@zkat
Copy link
Member

zkat commented Dec 6, 2017

@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.

@bmeck
Copy link
Author

bmeck commented Dec 6, 2017

@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).

@zkat
Copy link
Member

zkat commented Dec 6, 2017

@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.

@bmeck
Copy link
Author

bmeck commented Dec 6, 2017

@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.

@ljharb
Copy link

ljharb commented Dec 6, 2017

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.

@bmeck
Copy link
Author

bmeck commented Dec 6, 2017

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.

@justinfagnani
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants