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

new installation mode: isolated-mode #436

Closed
wants to merge 29 commits into from
Closed

new installation mode: isolated-mode #436

wants to merge 29 commits into from

Conversation

VincentBailly
Copy link

@VincentBailly VincentBailly commented Aug 23, 2021

This RFC is a proposal to add a new opt-in installation mode.

This mode would be the missing ingredient to make workspaces truly scalable.

Vincent Bailly and others added 2 commits August 23, 2021 12:51
@p01
Copy link

p01 commented Aug 23, 2021

Thank you!

accepted/0000-pure-mode.md Outdated Show resolved Hide resolved
Co-authored-by: Mathieu 'p01' Henri <mathieu@p01.org>
Copy link

@bnb bnb left a comment

Choose a reason for hiding this comment

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

Largely some small fixes, but there are a few questions. I would really love to see pure-mode available for non-workspaces both for consistency (creating two classes of DX isn't a good DX imo) and for potential perf gains (if this were implemented as a user-level package store, workspaces could also opt-in to that and see even better perf improvements in some situations), but if overall the npm CLI team and other RFC participants disagree I'm willing to back off of that.

I also see this as a path towards flexibility: if we have a pure-mode and a hoisted-mode, what's the reason we wouldn't add another mode when someone who is excessively smart finds yet another more optimized installation approach with good DX?

Overall, I'm very happy with this. Thank you for the hard work @VincentBailly <3

accepted/0000-pure-mode.md Outdated Show resolved Hide resolved
accepted/0000-pure-mode.md Outdated Show resolved Hide resolved
accepted/0000-pure-mode.md Outdated Show resolved Hide resolved
accepted/0000-pure-mode.md Outdated Show resolved Hide resolved
accepted/0000-pure-mode.md Outdated Show resolved Hide resolved
accepted/0000-pure-mode.md Outdated Show resolved Hide resolved
accepted/0000-pure-mode.md Outdated Show resolved Hide resolved
accepted/0000-pure-mode.md Outdated Show resolved Hide resolved
accepted/0000-pure-mode.md Outdated Show resolved Hide resolved
accepted/0000-pure-mode.md Outdated Show resolved Hide resolved
accepted/0000-pure-mode.md Outdated Show resolved Hide resolved
accepted/0000-pure-mode.md Outdated Show resolved Hide resolved
@darcyclarke darcyclarke added the Agenda will be discussed at the Open RFC call label Aug 24, 2021
accepted/0000-pure-mode.md Outdated Show resolved Hide resolved
accepted/0000-pure-mode.md Outdated Show resolved Hide resolved
accepted/0000-pure-mode.md Outdated Show resolved Hide resolved
accepted/0000-pure-mode.md Outdated Show resolved Hide resolved
accepted/0000-pure-mode.md Outdated Show resolved Hide resolved
accepted/0000-pure-mode.md Outdated Show resolved Hide resolved
@isaacs
Copy link
Contributor

isaacs commented Aug 25, 2021

So what happens in the cases where today you would get an ERESOLVE error?

Dependency graph:

root -> (a, b)
a -> PEER(c@1)
b -> PEER(c@2)

The current behavior of peer dependencies is that all dependencies at the same level will use the same version of a shared peer dependency. In the current (nested/hoisted/shared) mode, you'd get a conflict. However, it seems like with pure-mode, you'd get something like:

root
+-- a
|   +-- c@1
+-- b
    +-- c@2

(Via symlinks, of course, this tree is much simplified here from the actual fs result.)

I could see this, for example, resulting in multiple conflicting versions of react all trying to load at the same time. So it seems like either we call that out as a huge caveat, or figure out a way to raise the ERESOLVE in those cases.

accepted/0000-pure-mode.md Outdated Show resolved Hide resolved
accepted/0000-pure-mode.md Outdated Show resolved Hide resolved
accepted/0000-pure-mode.md Outdated Show resolved Hide resolved
@zkochan
Copy link

zkochan commented Aug 25, 2021

Some tools have issues with symlinks. By default, React Native does not like symlinks at all. Some hosting providers also don't like symlinks. Maybe if npm will use symlinks, they will add support.

Although this seems like an easy to implement feature, there are actually many edge cases. Do not underestimate it. Especially the peer dependency resolution part. And a lot of commands will need to be updated (at least install, update, and remove).

@fritzy
Copy link
Contributor

fritzy commented Aug 25, 2021

Action Item from today's Open RFC:

  • Answer: “What projects break today w/ pnpm?"
    • add that information back to the RFC
  • eslint can fail to resolve plugins if it is not directly installed as a dependency
  • packages tend to fail if they require a hoisted sub-dependency without realizing this -- might be a large list
  • Some tools don't like symlinks, like React Native, mentioned by @zkochan above.
  • Some "hosting providers" (I assume ci services or direct deploy static web services) don't like symbol links, again mentioned in @zkochan's comment.

Any other specifics?

@zkochan
Copy link

zkochan commented Aug 25, 2021

AWS Lambda has issues with symlinks. We suggest our users to bundle their code before deploying to Lambda.

Regarding solutions. We have these settings for workaround: hoist, hoist-pattern, public-hoist-pattern, shamefully-hoist

We have also package extensions, which was borrowed from Yarn, it allows to extend manifests of dependencies with missing packages.

.pnpmfile.cjs is more powerful but probably packageExtensions is enough in most cases.

And we also support Yarn's PnP, so pnpm can create a pnp.cjs file and a node_modules with no symlinks (symlink). PnP will help Node.js find all the packages even without the symlinks. But I guess PnP currently has even more issues with the ecosystem than symlinks.

@canonic-epicure
Copy link

canonic-epicure commented Aug 26, 2021

I'd like to add a consideration, that "pure" mode makes the location of the dependencies predictable and thus allows bundler-free/import map free web development. This is because the contract "all my dependencies are in my node_modules folder starts holding.

For example, currently, lets say my package has a dependency on package dependency. Lets say I want to import "web-way", as relative url:

import * from "../node_modules/dependency/index.js"

instead of the "node-way":

import * from "dependency"

But with "hoisting" mode, there's no guarantee the dependency will be there. With "pure" mode it is guaranteed.

I might be missing some edge cases, if so, please correct me.

Co-authored-by: isaacs <i@izs.me>
accepted/0000-pure-mode.md Outdated Show resolved Hide resolved
VincentBailly and others added 3 commits September 15, 2021 19:44
Co-authored-by: Tierney Cyren <accounts@bnb.im>
Co-authored-by: Jonathan Creamer <matrixhasyou2k4@gmail.com>
accepted/0042-isolated-mode.md Outdated Show resolved Hide resolved
accepted/0042-isolated-mode.md Show resolved Hide resolved
accepted/0042-isolated-mode.md Outdated Show resolved Hide resolved
accepted/0042-isolated-mode.md Outdated Show resolved Hide resolved
accepted/0042-isolated-mode.md Outdated Show resolved Hide resolved
accepted/0042-isolated-mode.md Outdated Show resolved Hide resolved
accepted/0042-isolated-mode.md Outdated Show resolved Hide resolved
accepted/0042-isolated-mode.md Show resolved Hide resolved
accepted/0042-isolated-mode.md Outdated Show resolved Hide resolved
accepted/0042-isolated-mode.md Outdated Show resolved Hide resolved
Copy link
Contributor

@fritzy fritzy left a comment

Choose a reason for hiding this comment

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

This is very good. I just added some comments to improve clarity and brevity.

accepted/0042-isolated-mode.md Outdated Show resolved Hide resolved
accepted/0042-isolated-mode.md Outdated Show resolved Hide resolved
- to make it possible for a package to import itself using its own name. For example, the following code `require.resolve("foo");` in `node_modules/.npm/foo@1.0.0-1234/node_modules/foo/bar.js` will correctly return `node_modules/.npm/foo@1.0.0-1234/node_modules/foo/index.js`.
- to prevent creating circular symlinks, even when packages have circular dependencies.

#### Peer dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can probably avoid a lot of this peerDep/edge case analysis if we specify that we are going to implement the isolation mode as a post-buildIdealTree conversion step.

That is, even though there are dependency graph shapes that may be enabled by isolated-mode, which are not enabled by hoisted mode, some some of these may be desirable, we are going to instead favor interoperability and reducing the degree of change in the dependency contracts enforced by npm.

So, whereas hoisted mode does the following:

  1. Calculate hoisted ideal tree to satisfy dependency graph.
  2. Reify ideal tree.
  3. Save ideal tree to package-lock.json

instead, npm will:

  1. Calculate hoisted ideal tree to satisfy dependency graph.
  2. Capture hoisted tree lockfile metadata.
  3. Convert hoisted tree into isolated tree.
  4. Reify isolated tree.
  5. Save hoisted tree lockfile metadata to package-lock.json

Thus, any situation not currently resolvable by hoisted-mode will not be possible with isolated-mode. However, this ensures that any project may be reified using either hoisted or isolated mode, or switched between them, without any changes to the project lockfile.

Anything that causes an ERESOLVE error in hoisted mode will also cause an ERESOLVE error in isolated mode.

Then the task is to document how that conversion process is accomplished. Imo, that ought to end up in this RFC document, even if it happens after some implementation work has been done (which is probably a good idea, since we're bound to find some edge cases we haven't thought of!)

Copy link
Contributor

Choose a reason for hiding this comment

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

@isaacs , can you make an edit or suggestion that reflects this?

Co-authored-by: isaacs <i@izs.me>
@fritzy fritzy changed the title new installation mode: pure-mode new installation mode: isolated-mode Sep 21, 2021
Updated node recommended links, added bikeshedding for the config option itself.
documented conversation about more aggressive symlinks in default hoisted mode
@isaacs isaacs closed this in 88542df Sep 21, 2021
@isaacs
Copy link
Contributor

isaacs commented Sep 21, 2021

Landed (with some formatting and our planned implementation approach within @npmcli/arborist) on 88542df.

Thank you all for getting this proposal to this point, most especially @VincentBailly, but also everyone else who contributed edits and discussion. This is a big one, and I think we whittled it down to the point where it provides the key pieces of value intended, while also being straightforward and feasible to implement, with a minimum of disruption for our users. I'm looking forward to getting this shipped!

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

Successfully merging this pull request may close these issues.