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

Package deduplication #350

Open
aleclarson opened this issue Jan 31, 2019 · 4 comments
Open

Package deduplication #350

aleclarson opened this issue Jan 31, 2019 · 4 comments

Comments

@aleclarson
Copy link
Contributor

aleclarson commented Jan 31, 2019

Is this a feature request? Yes

I often work on multiple packages at the same time. When two of these "locally developed" packages have the same exact dependency, I expect Metro to "deduplicate" the dependency instead of throwing.

Currently, the only workaround is to install everything into a flattened node_modules in the project root. This isn't practical, because I use symlinks during development (which is why I implemented #257).

@aleclarson
Copy link
Contributor Author

aleclarson commented Jan 31, 2019

When I disable throwOnModuleCollision here, the error moves to the following line in Metro:

if (error instanceof DuplicateHasteCandidatesError) {
throw new AmbiguousModuleResolutionError(fromModule.path, error);
}

This comment explains why using jest-haste-map to detect package "collisions" is a problem.

I propose the following behavior for package deduplication:
 

  1. When duplicate haste candidates are encountered...

    • Metro should mimic the Node.js resolution algorithm in order to determine which "duplicate" package path corresponds to the depending module.
       
  2. When one of the "duplicate" package paths is matched...

    • Metro should check which (if any) of the other "duplicate" package paths have already been resolved. For those package paths, check if one has an SHA1 hash that is identical to the matched path. Otherwise, use the matched path.
       

The developer is responsible for ensuring only one version of a package exists in the bundle. Metro should have no opinion on the matter.

Note: I wouldn't be against an option that tells Metro to emit warnings when multiple versions of the same package exist, but it should definitely be opt-in.
 

@aleclarson
Copy link
Contributor Author

aleclarson commented Feb 1, 2019

Alternatively, we could add an option to jest-haste-map which skips reading of package.json files when building the haste map. This is probably the easiest, so I'm gonna take a whack at this one first. I'm not sure if this will fix the problem entirely, but it will definitely affect performance negatively.

aleclarson added a commit to aleclarson/metro that referenced this issue Feb 1, 2019
Packages with the same name/version pair are deduplicated.

Closes facebook#350
aleclarson added a commit to aleclarson/metro that referenced this issue Feb 1, 2019
Packages with the same name/version pair are deduplicated.

Closes facebook#350
aleclarson added a commit to aleclarson/metro that referenced this issue Feb 2, 2019
Packages with the same name/version pair are deduplicated.

Closes facebook#350
aleclarson added a commit to aleclarson/metro that referenced this issue Feb 2, 2019
Packages with the same name/version pair are deduplicated.

Closes facebook#350
@prerakd
Copy link

prerakd commented Aug 10, 2019

@aleclarson Any updates on this PR?

@aleclarson
Copy link
Contributor Author

@prerakd The PR at #353 works great in my local fork. Waiting on response from Facebook...

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

No branches or pull requests

2 participants