facebook / metro

🚇 The JavaScript bundler for React Native
https://metrobundler.dev
MIT License
5.24k stars 626 forks source link

Add support for resolving import subpaths #1302

Open jameslawson opened 4 months ago

jameslawson commented 4 months ago

Summary

PR to add support in metro-resolver for resolving import subpaths; the feature discussed in Issue #978 in that reads the"imports" key in package.json

Test plan

npm run test

After Landing

codecov-commenter commented 4 months ago

Codecov Report

Attention: Patch coverage is 62.50000% with 21 lines in your changes missing coverage. Please review.

Project coverage is 83.70%. Comparing base (41615c5) to head (ad604ca).

Files Patch % Lines
...ckages/metro-resolver/src/PackageImportsResolve.js 69.69% 10 Missing :warning:
packages/metro-resolver/src/resolve.js 60.00% 8 Missing :warning:
...solver/src/errors/PackageImportNotResolvedError.js 0.00% 3 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1302 +/- ## ========================================== - Coverage 83.81% 83.70% -0.11% ========================================== Files 207 209 +2 Lines 10887 10943 +56 Branches 2733 2752 +19 ========================================== + Hits 9125 9160 +35 - Misses 1762 1783 +21 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kraenhansen commented 4 months ago

Thanks for picking this up!

robhogan commented 1 month ago

We'll need to update the resolver algorithm docs as well, but I'm happy to take care of that when importing.

Beachman4 commented 4 weeks ago

Node allows you to alias to an external package, could you modify your implementation to allow that? Currently reduceExportMap throws an error because exports can't map to an external package.

robhogan commented 4 weeks ago

Node allows you to alias to an external package, could you modify your implementation to allow that? Currently reduceExportMap throws an error because exports can't map to an external package.

That's a great point, thanks for spotting that.

I think that can land as a follow-up though, I'm wary of holding this PR up further, but I've noted it as something that we need to do asap and before claiming support for package.json#imports.

Beachman4 commented 3 weeks ago

Node allows you to alias to an external package, could you modify your implementation to allow that? Currently reduceExportMap throws an error because exports can't map to an external package.

That's a great point, thanks for spotting that.

I think that can land as a follow-up though, I'm wary of holding this PR up further, but I've noted it as something that we need to do asap and before claiming support for package.json#imports.

Yeah I think that's fair.

Just one last comment based on my usage of this WIP feature:

When there are references to other packages in the imports field, the current implementation will short circuit and throw an error as it considers those values to be invalid, which they are until it is implemented, but I think it would be better to compile the list of invalid values and echo that to the user, but still continue. The reason being that there could be valid values further down the import list and I think it's ideal if those are considered and processed.

The reason I would suggest this, is lets say this is released. I then pull down that change and add a custom resolveRequest function to my config to handle the external packages until support is added, but since that short circuit is there, this won't process the "good" subpath imports.

For example:

test -> "test-pkg" <-- Currently short circuits because of this

internal -> "./src/internal/*"

jameslawson commented 1 week ago

@robhogan hey Rob, when you get a chance would you mind having another look at this PR. I've tried to address all comments previously raised; please consider it "Ready to Review". Thank you so much!