Add support for resolving import subpaths

[jameslawson]

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

• Update the resolver algorithm doc
• Add support for resolving external packages

[codecov-commenter]

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 ⚠️
packages/metro-resolver/src/resolve.js	60.00%	8 Missing ⚠️
...solver/src/errors/PackageImportNotResolvedError.js	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kraenhansen]

Thanks for picking this up!

[robhogan]

Huge thanks for working on this @jameslawson and sorry for the lack of response from us.

There have been a lot of moving parts in the resolver recently and this would've been a breaking change we couldn't immediately pull in, but there's now room for it and a new, stable fileSystemLookup to use.

If you're happy to rebase, this is looking really close.

[robhogan]

True but nonetheless, using the "exports" name here is surprising. Before this PR they're private functions so there's no need to preserve the names.

I think I'd rename the functions to "subpaths map" or something as you suggested with @kraenhansen, and move them into a common location (probably under utils).

[jameslawson]

• Regarding the naming, I think it's a little tricky. I noticed a lot of comments already written by the team call these imports/export types: "'exports'-like", and I can't think of a better name. To me this feels the most natural name, probably because Node.js docs seems to skirt around giving them a name and just refers to them by pointing to the "exports" field.
• Nevertheless, I've renamed 3 functions and the ExportMap type to reflect that they are "'exports'-like" in order to indicate that they work for both importing with "imports" and exporting with "exports". Following your feedback, I've moved these functions to utils.

Let me know your thoughts!

[robhogan]

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

[Beachman4]

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]

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]

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]

@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!