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 Exports resolution differs from Node in 2 edge cases #1236

Open
jbroma opened this issue Mar 19, 2024 · 3 comments
Open

Package Exports resolution differs from Node in 2 edge cases #1236

jbroma opened this issue Mar 19, 2024 · 3 comments
Assignees

Comments

@jbroma
Copy link
Contributor

jbroma commented Mar 19, 2024

Do you want to request a feature or report a bug?
Bug

Hey metro team! 👋

I was working on aligning Re.Pack's resolution setup to follow metro-resolver with RN defaults closely and found these 2 edge-cases were metro-resolver behaves in a weird way when dealing with Package Exports:

1. Root Array Shorthand

For the Root Array shorthand, the implementation differs when compared to Node.js or Webpack's enhanced-resolve specification:

{
  "name": "@metro-package-exports/root-shorthand",
  "exports": ["./index.js"]
}

Results:

For the import specifier: @metro-package-exports/root-shorthand

  • Node.js: Resolves to index.js inside of @metro-package-exports/root-shorthand.
  • enhanced-resolve: Identically resolves to index.js within @metro-package-exports/root-shorthand.
  • metro-resolver: Fails to resolve, falling back to file resolution and ultimately resolving to the @metro-package-exports/root-shorthand directory itself.

2. Subpath Patterns in exports

When using subpath patterns in exports to match the import specifier, the behaviors observed are as follows:

{
  "name": "@metro-package-exports/restricted",
  "exports": {
    ".": "./index.js",
    "./features/*.js": "./features/*.js",
    "./features/internal/*.js": null
  }
}

Results:

For the import specifier: @metro-package-exports/restricted/features/internal/b.js

  • Node.js: Fails to resolve as b.js is not exported from @metro-package-exports/restricted.
  • enhanced-resolve: Also fails to resolve for the same reason as Node.js.
  • metro-resolver: Successfully resolves to @metro-package-exports/restricted/features/internal/b.js without falling back to file resolution.

Reproduction repository

https://github.com/jbroma/metro-package-exports

Setup

  1. Install dependencies via yarn v1 (classic).
  2. Make the test scripts within the tester directory executable by running chmod +x on them.
  3. Execute test-root-shorthand.sh & test-restricted.sh scripts.

What is the expected behavior?

Resolution in edge cases above should follow Node.js resolution algorithm

@jbroma
Copy link
Contributor Author

jbroma commented Mar 20, 2024

Few other insights about Package Exports implementation & tests:

  1. When multiple entires in root array shorthand are defined the resolvers behave in a following way:

    • node: looks for first valid specifier (beginning with ./), and returns it, even when it's non-existent
    • enhanced-resolve: similar to node, but looks for first valid entry that actually exists
    • metro-resolver: can't tell because case from the issue fails
  2. IMO, these test cases should be marked as [nonstrict] as they rely on fallback mechanism:

  • should resolve "exports" target directly > without expanding sourceExts
  • should resolve "exports" target directly > without expanding platform-specific extensions

FYI, im willing to submit a PR for this after we agree on the spec.

@huntie
Copy link
Member

huntie commented Mar 20, 2024

Thanks @jbroma!

1. Root Array Shorthand

Wow, essentially these implementations have a special-case expansion of ["./index.js"] to {".": "./index.js"} — happy to align ✅.

2. Subpath Patterns in exports

Yup, also looks incorrect ✅. I don't think we have a test against that behaviour at all.


IMO, these test cases should be marked as [nonstrict] as they rely on fallback mechanism:

  • should resolve "exports" target directly > without expanding sourceExts
  • should resolve "exports" target directly > without expanding platform-specific extensions

These tests are for in-spec behaviour, so I think this is right as-is. However, the assertions seem slightly wrong (unless I'm misremembering) — they should be index-exports.js. This test needs a little fixing.


Yes please to PRs! 🙌🏻 Recommend splitting into one PR per fix, and we'll happily look through :).

@jbroma
Copy link
Contributor Author

jbroma commented Mar 21, 2024

@huntie all tasks from the issue handled in the PRs above.

facebook-github-bot pushed a commit that referenced this issue Mar 27, 2024
Summary:
This PR deals with restricting internal paths through the most specific subpath pattern edge case from #1236

- [x] - added test for implicit default condition to match Node.js resolution algorithm
- [x] - aligned implementation of `reduceConditionalExport` to account for implicit default condition

Changelog: [Experimental] Fix implicit default condition to be null for subpath patterns (edge case)

Pull Request resolved: #1239

Test Plan: - [x] - all tests pass

Reviewed By: EdmondChuiHW

Differential Revision: D55244268

Pulled By: huntie

fbshipit-source-id: e9cddbad2709a90a0282385a1b59ebc03735f975
facebook-github-bot pushed a commit that referenced this issue Mar 28, 2024
…ve exports target directly tests (#1240)

Summary:
This PR adds comments to assertions in tests that check that neither `sourceExts` or `platformExts` are expanded when matching exports target directly, which was suggested inside of #1236 by huntie. The assertions rely on using `[nonstrict]` fallback mechanism as there is no other way to test this functionality right now because `strict` mode is not implemented.

Changelog: [Internal] Refactor Package Exports `sourceExts` and `platformExts` tests that rely on fallback

Pull Request resolved: #1240

Test Plan: - [x] - all tests pass

Reviewed By: hoxyq

Differential Revision: D55427364

Pulled By: huntie

fbshipit-source-id: 8a36469508c67406e721b0300383d6b18add3ad2
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