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: package deduplication #353

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

aleclarson
Copy link
Contributor

@aleclarson aleclarson commented Feb 1, 2019

Summary

This PR is a follow-up to #350. It deduplicates packages with the same name/version pair. 🎉

  • Disable throwOnModuleCollision for jest-haste-map
  • Enable skipHastePackages for jest-haste-map (feat: add "skipHastePackages" option jestjs/jest#7778)
  • Remove the resolveHastePackage function
  • Change ModuleCache#getPackage to use an existing package if it has the same name and version
  • Change ModuleCache#processFileChange to update the "name@version" => package map
  • Various refactoring

Only e29c485 is relevant to this PR. The others are from #257.

Test plan

No time to write tests, currently. 😢

I've tested this manually. Works great so far! 😃

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 1, 2019
@aleclarson aleclarson force-pushed the dedupe branch 2 times, most recently from 65b2a39 to e29c485 Compare February 2, 2019 01:30
- resolve symlinks inside "node_modules" directories
- add `follow` function to `ResolutionContext` type
- move "node_modules" logic into the `yieldPotentialPaths` function
- feat: scope-only keys in `extraNodeModules` map
- fix: always preserve scope when checking `extraNodeModules` map
- fix: throw an error if `resolveRequest` fails to resolve a direct import
- [BREAKING] refactor the `FailedToResolveNameError` class

This commit adds support for symlinks in any `node_modules` directory. This includes scoped packages. Also, PNPM users rejoice!

The `yieldPotentialPaths` function avoids extra work by lazily generating the potential module paths for indirect imports. Also, the resolver now skips over module paths that contain `/node_modules/node_modules/`.

The `extraNodeModules` map now has support for keys like `@babel` to act as a fallback for all modules starting with `@babel/`.

Previously, if the `resolveRequest` function failed to resolve a direct import, we would continue onto the "node_modules" search logic. The correct behavior is to throw an error.
We cannot assume that `filePath` is _not_ a directory.
Packages with the same name/version pair are deduplicated.
@aleclarson
Copy link
Contributor Author

aleclarson commented Jun 1, 2020

Rebased onto v0.56

An example "package import" is `require("foo")`

Contrast that with a "file import" like `require("foo/bar")`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants