fix(metro-resolver): package exports array root shorthand

jbroma commented 8 months ago

Summary

This PR deals with array root shorthand edge case from #1236

[x] - fixed array root shorthand spec in tests to match Node.js resolution algorithm
[x] - aligned implementation of package exports for array root shorthand
[x] - added extra [nonstrict] test case to highlight that non-declared paths from array should not work normally

Changelog: [Experimental] Fix package exports array root shorthand resolution to behave like node (edge case)

Test plan

[x] - all tests pass

codecov-commenter commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 83.58%. Comparing base (f8f7d55) to head (bab106d).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #1238 +/- ## ======================================= Coverage 83.58% 83.58% ======================================= Files 207 207 Lines 10737 10737 Branches 2683 2684 +1 ======================================= Hits 8974 8974 Misses 1763 1763 ```

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

facebook-github-bot commented 8 months ago

@huntie has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

robhogan commented 8 months ago

Huge thanks for working on this @jbroma.

From my testing just now I think there may be a bit more to this though that explains why an array is useful. It seems Node takes the first entry satisfying the asserted conditions.

Eg, with:

{
    "name": "pkg",
    "exports": [{"custom": "./foo.js"}, {"import": "./bar.js"}, "./baz.js"]
}

 resolve-test  node --conditions=custom -p 'require.resolve("pkg")'
/Users/robhogan/workspace/resolve-test/node_modules/pkg/foo.js
 resolve-test  node --experimental-default-type=module -e 'console.log(import.meta.resolve("pkg"))'
file:///Users/robhogan/workspace/resolve-test/node_modules/pkg/bar.js
 resolve-test  node -p 'require.resolve("pkg")'
/Users/robhogan/workspace/resolve-test/node_modules/pkg/baz.js

So I think the approach here would be to normalise to the equivalent object form, which I think is..

{
  ".":  {
    "custom": "./foo.js",
    "import": "./bar.js",
    "default": "./baz.js"
  }
}

That needs a bit more verifcation against other resolvers though, and we might also want to consider nested arrays or more complex objects, although I think correctly dealing with the array of strings-or-flat-objects case would be a strict improvement on where we are.

jbroma commented 7 months ago

@robhogan oh I absolutely agree - there is more to this than what meets the eye :)

I think we should proceed with merging this edge case and handle other scenarios in separate issues / PR's. The normalisation that you've provided seems like a good starting point to begin with. We can reuse the repro from the #1236 and just add more cases there.

Is there anything else that needs to be addressed in this PR? I see the FB internal job failed for some reason.

jbroma commented 5 months ago

@huntie @robhogan is there anything blocking us from proceeding with this PR?

facebook-github-bot commented 5 months ago

@huntie merged this pull request in facebook/metro@fa17a0d67c4ed06bb9347ee7d902147fa3893ef7.

facebook / metro