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

feat: add "skipHastePackages" option #7778

Merged
merged 1 commit into from
Feb 28, 2019

Conversation

aleclarson
Copy link
Contributor

Summary

The skipHastePackages option prevents reading of package.json files and keeps them out of the module map. It's important to fixing package deduplication in Metro (see here: facebook/metro#350), especially after symlink support is merged (see here: facebook/metro#257).

With support for symlinks in node_modules, it's very easy for the dependencies of one "locally developed" package to collide with another package's dependencies, which results in frustrating errors that are entirely avoidable.

Test plan

I tested it manually. Works great! I don't have time to get familiar with your test suite.

@SimenB
Copy link
Member

SimenB commented Feb 2, 2019

This is another one for the FB people to review, sorry about that 😅

(it'll need a changelog entry, though)

@mjesun
Copy link
Contributor

mjesun commented Feb 2, 2019

👍LGTM! We'll use this in Metro too. cc @rafeca (we recently discussed about it).

@cpojer
Copy link
Member

cpojer commented Feb 4, 2019

@mjesun how were you thinking of using this in Metro? We depend on this behavior at FB. Did you mean to disable this by default for open source?

@rafeca
Copy link
Contributor

rafeca commented Feb 4, 2019

@mjesun how were you thinking of using this in Metro? We depend on this behavior at FB. Did you mean to disable this by default for open source?

Some people using Metro internally at FB (but outside of RN) have run into similar issues due to global packages duplications (/cc @tjfryan ), allowing Metro to ignore haste packages would simplify their setups.

@aleclarson
Copy link
Contributor Author

@cpojer This option is false by default, so it shouldn't affect FB internals. LGTM?

Copy link
Member

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Could you rebase this now that we are using TypeScript?

@SimenB
Copy link
Member

SimenB commented Feb 25, 2019

I don't think the name skipHastePackages really says that it's just about package.json. Why not skipPackageJson or something?

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Missing changelog entry (and I think the name of the option is imprecise)

@aleclarson
Copy link
Contributor Author

Good to go 👍

Copy link
Member

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Thank you.

@cpojer cpojer merged commit df3eb5e into jestjs:master Feb 28, 2019
@AmirBosch
Copy link

how do i use this flag ?

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants