nohoist RFC

[connectdotz]

addressing #3882

[bestander]

Conceptually seems legit and reasonable.
Thanks for sending a very well thought through RFC, @connectdotz!

@BYK and @arcanis I'll leave the details review to you.

[connectdotz]

@bestander I just updated the RFC to callout nohoist exceptions for linked packages (including workspace itself): basically linked packages will always be hoisted. Please let me know if you see any issue with that.

Not sure what is the expected end result for an RFC... do we need explicit approval before submitting the code change PR? I assume the state of the RFC is still pending on @BYK and @arcanis review?

[bestander]

@connectdotz that is some rad RFC you've sent!
Great work and a pleasure to read.

Go ahead and submit a tentative implementation PR.
I think we can merge this as is but I'll give a few days for @BYK and @arcanis to veto if they have any other plans

[bestander]

nice approach!
Simplicity is important

[bestander]

I am not a huge fan of a new keyword.
Would it really be simpler than a glob?

Considering that we use globs already I think it would be expected by final users that nohoist fields is a glob too

[connectdotz]

@bestander I see your point, yes we can indeed use glob pattern to make it consistent with the packages patterns.

BTW, I loved the "why" feature of yarn, however, while using it to diagnose nohoist, there are a few bugs surfaced, mainly the current implementation assumed a given package only appeared once, which is not always true: 1. when version conflict 2. when it is marked nohoist. I will address them also in this PR so people can easily find out why a package appeared in multiple places.

(There is another why.js bug that it didn't look at peerDependency, but I will not include it in this PR, as it is independent of nohoist, trying to keep the scope tight.)

[bestander]

That is a good argument.
The counter argument is if we release it now and it goes to public packages then we won't have a chance to change our minds.
Especially if other package managers will implement workspaces.

How about this - we implement it the safer way, nohoist only in root but we can implement the nohoist in public packages behind an experimental flag name nohoist-yarn-experimental.
Give it some time and then decide how to proceed.

[connectdotz]

ok, in that case, we can just use the existing workspaces-experimental flag, since nohoist is within the workspaces config anyway, probably don't need another flag for nohoist explicitly.

Basically, both package producers and consumers need to have workspaces-experimental flag on in order to access any workspaces config/functionality.

[bestander]

Nope, unit tests should all pass.
Submit a PR and let's see what CI says

[bestander]

I understand that nohoist only works for workspace boundary, regular packages should not inherit workspaces' logic for hoisting.
So yeah, option-1 looks the right one

[connectdotz]

@bestander

regular packages should not inherit workspaces' logic for hoisting

If by regular packages you mean the project without turning on workspaces-experimental, then yes nohoist only impacts the hoisting logic via workspaces.nohoist in package.json.

The more I think about the original question raised in the RFC, the more I realize that it has to be option-1. option-2 is not a valid alternative actually. Because a hoisted package can be "originated" from multiple sources that might have conflicting nohoist rules, therefore, inheritance-from-source is problematic.

This also helps us simplify nohoist-inheritance concept, as it simply traverses the final node_module graph, just like package resolution.

I will update the RFC to reflect this.

[brunolemos]

Is this available as a beta or something like that?
Need to make react native command work.

[connectdotz]

quick update:

last week was a busy one but I managed to spend the weekend on this. Finally able to use the new nohoist for a fresh react-native run in my sandbox workspaces... Bumped into quite a few incompatible hidden assumptions in the non-hoisting part of the system and discovered that we do need deep-nohoist for react-native (Metro bundler etc)... anyway, I should be able to submit the code PR this week so we can have more concrete discussions and more people testing it...

had to change the design a bit to provide a more powerful/flexible deep-nohoist mechanism. Fortunately, @bestander suggestion of using glob pattern matching provided an intuitive way to do this. I will need to update this RFC as well...

[connectdotz]

@bestander @BYK @arcanis would love to hear your thought on this issue...

[bestander]

Thanks, I'll check this later today or tomorrow

[bestander]

That is a very thorough analysis, @connectdotz!
I agree with the proposal, let's get it in.

I'll ping the team to comment before merging in a couple of days.

[arcanis]

@connectdotz Great job on this RFC! My feedback:

• A separate flag from workspaces-experimental will be needed, since this flag now defaults to true and will be removed in a few releases from now.

• I feel strongly against allowing the nohoist option in non-private packages (they don't need to be workspace roots, but they need to be marked as private: true). Doing this would remove users the control over the configuration of their package managers.

• As for the dependency tree representation, we should use the same one than the one used for the selective version resolution feature. Which is great, since it's very similar to the syntax you came up with :)

[connectdotz]

@bestander @arcanis thx for the feedback, addressing @arcanis comments:

A separate flag from workspaces-experimental will be needed, since this flag now defaults to true and will be removed in a few releases from now.

I see, then it does make sense to have a new one. I will add workspaces-nohoist-experimental opt-out option.

feel strongly against allowing the nohoist option in non-private packages (they don't need to be workspace roots, but they need to be marked as private: true). Doing this would remove users the control over the configuration of their package managers.

I wasn't quite sure what you refer to as "Doing this would remove users the control over the configuration of their package managers." Each package, public or private, is indeed giving package manager instruction on what to load. It is true that they usually don't care where it should be loaded, but for some cases that the position of the module is critical like the react-native module, somebody has to specify the exception. We can ask each consumer (i.e. our projects) to mark the exception, or the package owner can mark the exception itself. I think the later is more efficient and scalable.

If you are concerning malicious "take over", I should have documented it better that nohoist rule is always prefixed with the "dependency path" at runtime, so they can really only influence their own packages tree down. For example: in a A/B/C tree, even if B has a [**"] nohoist rule, it will only impact any package match A/B/**, so none of A's other dependency will be impacted by B, i.e. B can only impact its own dependencies' hoisting strategy.

If for whatever reason we need to turn off a public package's nohoist rule, the workspace can always use glob's negation operation to override package's. (I didn't implement this in the initial PR as it can make the nohoist syntax a bit messy, but we can easily enable it later, if need to.)

Can a public package do stupid thing? Yes, they could do many stupid things like adding unnecessary dependencies or adding unnecessary nohoist rules. They can cause our workspace less efficient but will most likely not break it. However, if the package didn't specify a needed nohoist rule, our workspace will not work. So my thinking goes like this: 1) there are reasonable protections to sandbox the nohoist rules to its originating package or override it; 2) abusing the rule causing less harm than not being able to specify it... therefore, nohoist for public package should be considered and even encouraged if the package requires it to work...;-)

As for the dependency tree representation, we should use the same one than the one used for the selective version resolution feature. Which is great, since it's very similar to the syntax you came up with :)

Just to clarify... you are saying 'yes we should use path-like syntax to express dependency tree', right? I think we all agree that for configuration, glob pattern is the one to use, such as in workspaces.packages, version-selection and now nohoist.

Sorry for not being clear, what I meant to ask is specifically regarding reporting progress and state, such as during install or why. For example when asking yarn why is-finite in a nohoist react-native workspace, it (PR #4979) will produce the following result:

$ yarn why is-finite
yarn why v1.3.2
[1/4] 🤔  Why do we have the module "is-finite"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
=> Found "[email protected]"
info Reasons this module exists
   - "/_project_/ites/ts-jest/babel-core/babel-generator/detect-indent/repeating" depends on it
   - Hoisted from "repeating#is-finite"
info Disk size without dependencies: "16kB"
info Disk size with unique dependencies: "32kB"
info Disk size with transitive dependencies: "32kB"
info Number of shared dependencies: 1
=> Found "ites#[email protected]"
info Reasons this module exists
   - "/_project_/ites/react-native/metro-bundler/babel-generator/detect-indent/repeating" depends on it
   - Hoisted from "ites#repeating#is-finite"
   - in the nohoist list ["/_project_/**/react-native","/_project_/**/react-native/**"]
info Disk size without dependencies: "16kB"
info Disk size with unique dependencies: "32kB"
info Disk size with transitive dependencies: "32kB"
info Number of shared dependencies: 1
✨  Done in 1.71s.

You can see there are mixed dependency tree expressions. The new ones use standard path-like format('/'), the rest use yarn custom format ('#'). We should be consistent, if possible. IMHO, if there is no particular reason in choosing '#', we should probably consider standardizing on the path-like format instead...

[arcanis]

Each package, public or private, is indeed giving package manager instruction on what to load.

Yes, but they don't give the package manager instructions on how to store it on the filesystem. Two actual examples:

• The dependencies lockfiles are ignored - only the top-level one counts

• There is no way for a dependency to specify that it should be installed in flat mode

We can ask each consumer (i.e. our projects) to mark the exception, or the package owner can mark the exception itself. I think the later is more efficient and scalable.

I see your point, but they're other factors into play:

• Adding this feature to the package level encourages package developers to rely on those mechanisms, which are broken imo (in that they prevent the package manager from actually doing its job efficiently). Requiring this feature to be described in the top-level package.json makes it clearer that relying on the hoisting is not a pattern we want you to use, while unblocking the few packages that actually need it.

• Adding this feature to the package would be simultaneously too much and not enough: too much because it would silently affects the file tree in quite unpredictable ways, and not enough in the sense that if a package makes assumption on the hoisting of its dependencies, it also makes sense to think that it will also make such assumptions on the hoisting of its siblings (which it wouldn't be able to nohoist, per the rule you detailed above).

I'm not so concerned about malicious packages, however I'm concerned about the ecosystem, and I prefer not to encourage a pattern that effectively decreases the user experience by making the node_modules larger and larger, and installs slower and slower.

We should be consistent, if possible. IMHO, if there is no particular reason in choosing '#', we should probably consider standardizing on the path-like format instead...

Oh, I see. Yes, I agree it would be better to be consistent. However, I think this is worth being discussed in a separate PR - I'd suggest to use # for the time being and to revisit the output format in a followup issues.

[connectdotz]

@arcanis I think we might have misaligned on a very important assumption: nohoist is not a stand-alone 'feature' for packages to take-over location decision from PM, it is merely a 'workaround' to make an otherwise 'working' package function within the monorepo project. nohoist is an no-op outside of workspaces. The developers have to make their packages work without nohoist (most of their consumers will not in monorepo env), and only add nohoist if some of its dependency hasn't been updated to work in monorepo env (we can easily add a warning to make it clear that such package should be fixed soon). In other words, There is no incentive for developers to add unnecessary nohoist.

If this is still not enough to make you feel comfortable, then let's just make it private only as you and @bestander suggested😉 . Since many workspaces are also public packages, this change will mean that only the root package.json can specify nohoist. I think it is probably all right for now. We are just debating on some hypothetical situations that we might not yet fully understand anyway... If we ever change our mind, it is literally a 1-line code change...

I'd suggest to use # for the time being and to revisit the output format in a followup issues.

hmmm... ok

[arcanis]

There is no incentive for developers to add unnecessary nohoist.

Eh eh, I'm afraid if it worked like that no package would have ever relied on the hoisting mechanism in the first place :p

Ok, let's do the private thing, and I'll go review your implementation PRs!

[connectdotz]

@bestander @arcanis code changes and RFC all updated per our discussion

[arcanis]

LGTM, thanks!