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

Workspaces - Is it possible to exclude some node_modules from beeing hoisted to the root node_modules folder? #3882

Open
thomasfr opened this issue Jul 10, 2017 · 41 comments

Comments

@thomasfr
Copy link

thomasfr commented Jul 10, 2017

Do you want to request a feature or report a bug?
Feature

What is the current behavior?
We are currently using hoist feature from lerna. I tried to move to yarn workspaces and it works way faster! Awesome, thank you!

If the current behavior is a bug, please provide the steps to reproduce.

What is the expected behavior?
But i also need the ability (sadly) to exclude some node_modules from not beeing hoisted to the root node_modules folder but be physically installed in the package i configured it as dependency. (The same way the --no-hoist arguments are working for lerna. )
The reason is, sadly, that some dependencies we use, are trying to require some dependencies with an absolute path from packages/<package>/node_modules directly. So i have to exclude some.
Is there a way to configure some sort of exclude glob for workspaces?

Please mention your node.js, yarn and operating system version.
yarn --version: 0.27.5
node -v: v6.11.0
macOS: 10.12.5

@BYK
Copy link
Member

BYK commented Jul 10, 2017

@thomaschaaf is this a feature request? :) (since you haven't mentioned that in the first question)

@BYK
Copy link
Member

BYK commented Jul 10, 2017

//cc @bestander

@thomasfr
Copy link
Author

@BYK If its not yet possible (maybe it is not documented?) than it is a feature request.

@bestander
Copy link
Member

Would a symlink to the hoisted location work?

@thomasfr
Copy link
Author

lerna installs it directly when using --no-hoist. But i think a symlink should also work, i can try that out later. I will let you know

@seansfkelley
Copy link

Another use-case for disabling hoisting is that it helps prevent devs from creating accidentally-malformed packages.

I worked on a Lerna repo that had hoisting enabled. A number of the packages were erroneously using hoisted dependencies they had not specified because the code compiled/ran fine due to the way in which Node searches for modules -- namely, by traversing up the file hierarchy until it finds something in any ancestor's node_modules with the right name. In turn, this meant that packages would be published broken, because consumers wouldn't have the complete, true list of dependencies at install time.

For our use-case, hoisting bought us nothing while introducing more pitfalls, so I would love a way to disable hoisting on any given monorepo -- indeed, I would actually argue that no-hoist should be the default for the reasons stated above and by the principle of least surprise.

@bestander
Copy link
Member

All good points here.
We can discuss whether the no-hoist should be default later.
For now I can't dedicate enough time to work on this feature and help is needed.

The hoisting logic is isolated in https://github.com/yarnpkg/yarn/blob/master/src/package-hoister.js and you know whether the dependency is a workspaces if it's name is workspaceLayout.virtualManifestName.
Send a PR

@connectdotz
Copy link
Contributor

we bump into this issue with react-native so working on this PR. Question regarding the preference of where to put the nohoist configuration. A few options to consider:

  1. put it in projects.json, like the workspaces, create a top-level nohoist:[] property
  2. put it in projects.json, but create a new namespace, such as yarn or workspaces, and put all workspaces related info there, including nohoist.
  3. put it in .yarnrc (not sure about this because it seems .yarnrc is for yarn itself, but leave it here for completion)

Let me know if you see other better options, right now we are leaning toward option 2, suggestion something like this:

//package.json
workspace: {
    workspaces:[],
    nohoist:[]
}

thoughts?

@andrewmcwatters
Copy link

andrewmcwatters commented Oct 19, 2017

At AAA, we have some packages that still depend on Grunt and grunt.loadNpmTasks looks at only the relative node_modules directory. Additionally, grunt-contrib-jasmine has an options.vendor parameter that uses relative directories.

Having nohoist available in package.json/workspaces/nohoist would help us out a lot.

package.json/workspaces:array

"workspaces": [
    "packages/*"
]

package.json/workspaces:object

"workspaces": {
  "packages": [
    "packages/*"
  ],
  "nohoist": [
    "grunt*",
    "jasmine*"
  ]
}

@connectdotz
Copy link
Contributor

@andrewmcwatters , I like it

@connectdotz
Copy link
Contributor

connectdotz commented Oct 22, 2017

[discussion]
while working on the PR, I bump into the following package dependency scenario that could have multiple choices of implementations: if A marks B as nohoist, assuming no version conflict, what should the final graph be like?

A -> B -> C -> A
C

1. shallow nohoist
will try to hoist as much as possible, so only B is left under A, all B's dependency are hoist as usual. nohoist is not inheriented.

A -> B
C

2. deep nohoist
all of B's dependency will be treated as nohoist as well, (except to break the circular reference). nohoist is inherited.

A -> B -> C-> A
C

3. hybrid (weak inheritance)
Like deep-nohoist, this will assume all B's dependencies are nohoist, however, like shallow-hoist, these packages are capable to be hoisted up if they are referenced more than once. Lack of a better term, let's just called this "weak-inheritance". Since C appeared only once in the dependency chain, so it acts as nohoist; but C->A is broken because A occurred multiple times so it will defer to the hoisted version.

A -> B -> C
C

What do you guys think?

@connectdotz
Copy link
Contributor

@bestander which option is more aligned with the yarn philosophy? @andrewmcwatters, any of the options above will not work for your use case?

@DannyvanderJagt
Copy link

DannyvanderJagt commented Oct 23, 2017

We run also into this with react-native and I created my own script for now. Started to do some experiments and I think it would be great if the developer also could specify the nohoist for each /package/[name]. This will provide the most flexibility and will provide a solution for a lot of use & edge cases.

Our use case:

  • In my react-native package it is required to have the module react-native and react-native-navigation in the package/[name]/node_modules because of xcode (xcode doesn't handle symlinks well). The rest of the node_modules can be hoisted because they will be collected by the packager server (does handle symlinks).

The structure can be:

- node_modules (hoisted modules)
- packages
  - react-native-project
      - node_modules (nohoist modules: react-native, react-navigation etc)
  - other-package-project
- package.json

Suggestions:

  • We could specify a nohoist property in the project.json/root package.json like @connectdotz suggested with option 2. This is more like a global (applying to all the workspaces) kind of solution. But this doesn't allow for customization for each /package/[name]
  • We could also allow to specify a nohoist in the package.json file of a /package/[name]/package.json. This will provide customization for each /package/[name]

I think it would be nice to combine the solution and provide both. I am interested in your opinions and user/edge cases!

@connectdotz
Copy link
Contributor

@DannyvanderJagt thanks for the feedback, yes the nohoist will be available in any package.json, not just the root.

It is pretty clear how to make react-native itself nohoist, the question is more about how we should handle react-native's dependency. Use your scenario as an example, let's assume your react-native-project uses jest and so does the react-native:

  1. shallow-nohoist: you might end up with an empty react-native/node_modules
- node_modules (hoisted modules, including jest)
- packages
  - react-native-project
      - node_modules
          - react-native
             - node_modules
                (empty)
  - other-package-project
  1. deep-nohoist: most react-native's dependency will stay, duplicate packages might occur (like jest)
- node_modules (hoisted modules, include jest)
- packages
  - react-native-project
      - node_modules
          - react-native
             - node_modules
                - jest
                   - node_modules
                      (all of jest's depedency ...)
                - (other react-native's dependencies and their dependencies...) 
  - other-package-project
  1. hybrid (weak inheritance): You will NOT see jest under react-native/node_modules because it will be hoisted up in the root node_modules; however, all the packages unique to react-native will stay in the react-native/node_modules.
- node_modules (hoisted modules, including jest)
- packages
  - react-native-project
      - node_modules
          - react-native
             - node_modules
                - (other react-native's dependencies and their dependencies...) 
  - other-package-project

(I updated the earlier comment to make it more clear. )

@connectdotz
Copy link
Contributor

the more I think about it, the more I think maybe the deep-nohoist is the safest way to go. It is definitely not space-efficient, but since nohoist is mainly a workaround for packages that are not using the normal package management facility, a trade-off in space-efficiency is probably acceptable.

Ideally, react-native and others should be updated to use the package manager and not walking the file tree itself. Until then, we use nohoist as a workaround...

please let me know if you disagree with this approach.

(I will be travelling this week, but should have some time next week to finish it)

@connectdotz
Copy link
Contributor

connectdotz commented Oct 23, 2017

Maybe we could do better... 😏

just thought about something that might make shallow-hoist a better option: By default, every nohoist package is "shallow", if some package is looking to also nohoist a deep dependency, for example, jest under react-native, it could just add "jest" into the react-native-project's package.json:

workspaces = {
    nohoist = ["react-native, "jest"]
}

in this case, every "jest" package under react-native-project will also not be hoisted. The benefit is that yarn doesn't have to make assumption and developers can add as much or little nohoist packages as they need while maximizing the benefit of hoist.

Since the configuration will be position-aware, other projects used jest could still hoist jest, i.e. the "nohoist" will only apply to the dependency tree downward from where it is specified.

let me know if you see any logic flaw or missed use cases...

@DannyvanderJagt
Copy link

@connectdotz Thanks for the explanation! The last one looks solid.

Just quickly tested this for a react-native project by simulating a no hoist for only the react-native package and everything looks good.

@connectdotz
Copy link
Contributor

perfect! thx @DannyvanderJagt

@aaronbdixon
Copy link

Is there a possible ETA on when we could hope to merge this functionality in?

@bestander
Copy link
Member

bestander commented Oct 24, 2017

@connectdotz, I enjoy seeing so much enthusiasm around solving this issue!
Great work so far.
Please consider one aspect that not all users of npm registry use Yarn, there are quite a few package managers now and if you want to add the nohoist field for publishable packages you may be affecting the ecosystem.
A safer step would be to allow no-hoist only for private packages or workspace roots.
This way we will get feedback from people who opt-in to use this feature and leave a door open for breaking changes before advertising it among other package managers.

I think the nohoist feature can be very useful, similar to selective version resolutions.
Once you are ready to roll this out would you send the RFC for it?
I think more people would want to review the proposal.

As for your question #3882 (comment), I think shallow hoist (1) looks more consistent with what Yarn currently does.

@edmorley
Copy link
Contributor

edmorley commented Oct 25, 2017

Another use-case for disabling hoisting is that it helps prevent devs from creating accidentally-malformed packages.
...
A number of the packages were erroneously using hoisted dependencies they had not specified because the code compiled/ran fine due to the way in which Node searches for modules

@seansfkelley, the eslint-plugin-import no-extraneous-dependencies rule can help prevent this fwiw.

@OliverJAsh
Copy link

@edmorley

@seansfkelley, the eslint-plugin-import no-extraneous-dependencies rule can help prevent this fwiw.

Unfortunately this won't catch extraneous dependencies which are required implicitly, for example TypeScript typings (e.g. @types/node).

@BYK
Copy link
Member

BYK commented Oct 27, 2017

Related issues: #4219, #4099 and #4297.

@flaviouk
Copy link

Just ran into the same issue with react-native, do you guys have an ETA on this feature?

@connectdotz
Copy link
Contributor

sorry for the late reply, I was off-grid in the last few days (traveling until mid next week)

@bestander yes I will submit an RFC, probably should have started this discussion as an RFC... oh well...

For those who wondered about the progress of nohoist, I will be back to the state next week and plan to submit the RFC in the 1st week of Nov. Not sure how long the RFC process itself will take, but at least we shall see some concrete progress then... if you have the time and idea, I am sure you will be welcome to start the RFC process sooner...

@dariocravero
Copy link

dariocravero commented Dec 2, 2017

In case anyone is stuck with this and until the different issues are fixed, I made a little guide on how to use yarn workspaces with Create React App and Create React Native App (Expo) to share common code across. Hope you find it handy! https://learn.viewsdx.com/how-to-use-yarn-workspaces-with-create-react-app-and-create-react-native-app-expo-to-share-common-ea27bc4bad62 https://medium.com/viewsdx/how-to-use-yarn-workspaces-with-create-react-app-and-create-react-native-app-expo-to-share-common-ea27bc4bad62

@bestander
Copy link
Member

@dariocravero great writeup and thanks for investing your time in it!
Going to tweet about it

@Zerim
Copy link

Zerim commented Dec 14, 2017

Related issues: #4743 & #4503.

Another reason that hoisting seems problematic is that it could lead a library author to erroneously believe their package has the correct dependencies defined, when in fact they are just being resolved from hoisted dependencies of a sibling package. Wouldn't find out until after publishing that there was a problem.

@Zerim
Copy link

Zerim commented Dec 15, 2017

A hack to avoid specific dependency being hoisted seems to be to install a conflicting version of the dependency in the root directory, but this still doesn't address the issue mentioned above of not realizing that you're relying on hoisted modules.

@bradfordlemley
Copy link

Wondering if symlink'ing to the hoisted location, as suggested by @bestander in #3882 (comment), would just work by default for most of the use cases that are broken by the current implementation. Additionally this would allow that hoisted location (top-level 'node_modules') to be renamed to something like 'hoisted_modules', which would fix the issue of inadvertently picking up dependencies that are not defined by your package, as described above by @Zerim.

@AlexLandau
Copy link

@bradfordlemley There is another tool for npm monorepos called Rush that uses an approach similar to what you're describing -- installing shared dependencies in a folder not seen as a node_modules folder, then using symlinks to connect projects to their dependencies. (Their approach goes a bit further to allow multiple versions of a given module to be shared, further reducing the disk space and copying needed. In my testing, this also resulted in a faster webpack build for our application that used less memory.)

I'd be very happy if yarn workspaces offered a dependency layout on disk similar to the one used by Rush. It would fix all the hoisting-related issues I'm aware of.

@MrLoh
Copy link

MrLoh commented Jan 13, 2018

Great to see that there's an RFC. Really hope this will come to fruition in the near future, I'm also using react-native and am blocked from using yarn workspaces because of this issue at the moment.

@SEAPUNK
Copy link

SEAPUNK commented Jan 13, 2018

The base implementation of nohoist is already in a PR and mostly done, so I think we'll get a resolution for this soon enough.

@connectdotz
Copy link
Contributor

@thomasfr and everybody who has asked for this feature... in case you are not aware yet, nohoist has been released with yarn 1.5.1. Feel free to give it a try and let us know if there is any problem. Examples are also available here.

@morgs32
Copy link

morgs32 commented Apr 4, 2018

@connectdotz Is there a way to temporarily disable yarn (and lerna) hoisting in the case you want to do some final integration tests against published packages?

@connectdotz
Copy link
Contributor

not quite sure what you mean by "temporary"... here is how you can turn on/off nohoist at any time you wish:

  • to turn on nohoist: add nohoist config in your package.json, do a yarn install (add --force if needed)
  • to turn off nohoist: either remove the nohoist config or setting workspaces-nohoist-experimental to false, then run yarn install again (add --force if needed)

@RDeluxe
Copy link

RDeluxe commented May 9, 2018

I think he was thinking of a command such as yarn install --nohoist-all --force to have a clean layout.
It can be usefull for CI/CD.

@connectdotz
Copy link
Contributor

there is no command line option for nohoist currently. However, for CI/CD, you can have different .yarnrc, where you can set workspaces-nohoist-experimental to what you desire.

@morgs32
Copy link

morgs32 commented May 14, 2018

I apologize for the unspecific question. I actually meant can I disable all the hoisting AND LINKING, in order to do final integration tests against published packages. As such, it probably belongs in its own issue and not here because I don't think it relates to the nohoist option at all.

Admittedly, all this really tests is that in the process of publishing I did not accidentally .npmignore a required file. Or that all peer dependencies are installed... sounds a bit overkill but still I am tempted.

Perhaps I should phrase my question as a way to disable yarn (and probably lerna) altogether.

I'm not sure how to yarn install the actual packages off the registry unless I mutate the package.json and remove the workspaces prop. Because if I just use .yarnrc to turn the workspaces flag off, yarn yells at me for still having the workspaces prop in my package.json

Thoughts?

@IanVS
Copy link

IanVS commented Jan 23, 2020

It seems like with nohoist this issue can be closed now, right?

@humoyun91
Copy link

To someone who is curious about the problem of absolute paths and the necessity of nohoist option:

The problem with absolute path requirements in certain dependencies is that they can lead to issues with package resolution and portability. When a dependency specifies an absolute path to require other dependencies, it assumes a specific file system structure and location. This can cause problems when the package is used in different environments or by different developers who have different file system structures.

For example, let's say a package has a dependency that is required using an absolute path like "/packages/some-package/node_modules/some-dependency". If another developer or environment has a different file system structure, where the package is located in a different directory or has a different path, the absolute path requirement will not be valid and the dependency resolution will fail. Absolute path requirements can also make it harder to move or reorganize packages within a project, as it would require updating the absolute paths in all the affected dependencies.

In general, it is considered best practice to use relative paths or module resolution mechanisms (such as Node.js module resolution or module bundlers) instead of relying on absolute paths. This ensures that the dependencies can be resolved correctly in different environments and makes the project more portable and maintainable.

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

No branches or pull requests