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

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

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

Conversation

jbroma
Copy link
Contributor

@jbroma jbroma commented Mar 21, 2024

Summary

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

  • - fixed array root shorthand spec in tests to match Node.js resolution algorithm
  • - aligned implementation of package exports for array root shorthand
  • - 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

  • - all tests pass

@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 Mar 21, 2024
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Mar 21, 2024
@jbroma jbroma changed the title fix: package exports array root shorthand fix(metro-resolver): package exports array root shorthand Mar 21, 2024
@codecov-commenter
Copy link

codecov-commenter commented Mar 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@           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           

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

Copy link
Member

@huntie huntie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'm still trying to find where the previous root shorthand behaviour was documented. Lining up with current Node makes sense 👍🏻

@facebook-github-bot
Copy link
Contributor

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

@robhogan
Copy link
Contributor

robhogan commented Apr 3, 2024

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
Copy link
Contributor Author

jbroma commented Apr 8, 2024

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

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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants