facebook / metro

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

Use `resolvePackage` instead of `resolveModulePath` in `resolveHasteName` #1136

Closed kraenhansen closed 1 year ago

kraenhansen commented 1 year ago

Summary

This fixes #1128 by calling the resolvePackage instead of resolveModulePath in resolveHasteName. Only resolvePackage has the code to resolve package "exports" and it calls resolveModulePath as a fallback.

Test plan

I've added a failing test which passed as the fix got implemented.

codecov-commenter commented 1 year ago

Codecov Report

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

Comparison is base (e2d6419) 83.09% compared to head (3cef847) 83.09%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1136 +/- ## ======================================= Coverage 83.09% 83.09% ======================================= Files 206 206 Lines 10549 10549 Branches 2619 2619 ======================================= Hits 8766 8766 Misses 1783 1783 ``` | [Files](https://app.codecov.io/gh/facebook/metro/pull/1136?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=facebook) | Coverage Δ | | |---|---|---| | [packages/metro-resolver/src/resolve.js](https://app.codecov.io/gh/facebook/metro/pull/1136?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=facebook#diff-cGFja2FnZXMvbWV0cm8tcmVzb2x2ZXIvc3JjL3Jlc29sdmUuanM=) | `97.51% <100.00%> (ø)` | |

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

facebook-github-bot commented 1 year ago

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

robhogan commented 1 year ago

Thanks @kraenhansen - this LGTM (and @huntie), importing to run it through internal CI.

FWIW, enableHastePackages is default-off from Metro 0.79, so hopefully these cases of unintentionally resolving via Haste should go away, but this is a good correctness fix nonetheless.

facebook-github-bot commented 1 year ago

@robhogan merged this pull request in facebook/metro@7b1eb60676f0cfe22fcc224fd044fa3a7f2f94d9.

kraenhansen commented 9 months ago

I'm wondering if we need to update the documentation based on this change 🤔