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

Yarn ignores bundledDependencies and copies all node_modules for file: packages #4532

Closed
flarnie opened this issue Sep 23, 2017 · 6 comments
Closed

Comments

@flarnie
Copy link

flarnie commented Sep 23, 2017

Do you want to request a feature or report a bug?
This is a bug.

What is the current behavior?
In an example app from the Draft.js project, if I run yarn install I end up with both node_modules/react and node_modules/draft-js/node_modules/react. The second is a duplicate, and causes the "Refs must have an owner" warning

To compare - if I rm -rf node_modules && npm install in that same directory, I do not end up with node_modules/draft-js/node_modules/react.

If the current behavior is a bug, please provide the steps to reproduce.
If you clone the Draft.js github repo, yarn install && yarn build, and then follow the README in the universal example you should see the warning in the console and the node_modules situation I described above.

What is the expected behavior?

Draft.js lists react as a peerDependency, so I don't expect it to be installed when there is a sibling react module in node_modules.

Please mention your node.js, yarn and operating system version.
I just updated Yarn to 1.0.2 and used n to run node v4.8.4.

@gaearon
Copy link
Contributor

gaearon commented Sep 25, 2017

Can you reproduce with Yarn 1.1.0?

@BYK
Copy link
Member

BYK commented Sep 25, 2017

Most peerDependency issues should be resolved by #4478. That said I tried to replicate this locally and was able to reproduce it consistently because react is also listed as a devDependency under darft-js. Although ideally they'd be deduplicated, there's no guarantee that a dev dependency of darft-js will also be satisfied by its peer dependency. Also, they have different ranges too. I tried removing react and react-dom from devDependencies of draft-js and the duplication went away.

Do you think we can fix this somehow by not listing react in both places? Maybe something like #1503?

@gaearon
Copy link
Contributor

gaearon commented Sep 25, 2017

That said I tried to replicate this locally and was able to reproduce it consistently because react is also listed as a devDependency under darft-js.

Why does this matter though? devDependencies should only be installed when you run install inside of the package that specifies them. In my understanding, running install inside an example that depends on draft-js should not install devDependencies of draft-js.

@BYK
Copy link
Member

BYK commented Sep 25, 2017

I think this is due to the file: protocol but I need to investigate more.

@BYK
Copy link
Member

BYK commented Sep 26, 2017

So this is happening because when you use file: yarn simply copies the whole contents of that folder, including node_modules which comes with its own react dependency. Ideally, it would "pack" the folder on the fly and use it which means omitting node_modules unless there are bundled dependencies. I don't know how npm does it correctly since with npm 5, they actually changed the meaning of file: to be link: where it is impossible to avoid this.

@BYK BYK self-assigned this Sep 29, 2017
@BYK BYK added cat-bug and removed bug-linking labels Oct 27, 2017
@BYK BYK changed the title yarn install adds peerDependency twice in this example Yarn ignores bundledDependencies and copies all node_modules for file: packages Oct 30, 2017
@BYK
Copy link
Member

BYK commented Oct 30, 2017

Turns out this is a duplicate of #3344 so closing in favor of that.

@BYK BYK closed this as completed Oct 30, 2017
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

3 participants