facebook / metro

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

fix: support serverRoot with hmr serializer chunks #1137

Closed EvanBacon closed 1 year ago

EvanBacon commented 1 year ago

Summary

We use unstable_serverRoot heavily in Expo to support monorepos and consistently have issues where saving a file will log an error in the console:

Error: Unable to resolve module ./app/index from /Users/evanbacon/Documents/GitHub/expo/.: 

None of these files exist:
  * ../../app/index(.web.ts|.ts|.web.tsx|.tsx|.web.mjs|.mjs|.web.js|.js|.web.jsx|.jsx|.web.json|.json|.web.cjs|.cjs|.web.scss|.scss|.web.sass|.sass|.web.css|.css|.web.cjs|.cjs)
  * ../../app/index/index(.web.ts|.ts|.web.tsx|.tsx|.web.mjs|.mjs|.web.js|.js|.web.jsx|.jsx|.web.json|.json|.web.cjs|.cjs|.web.scss|.scss|.web.sass|.sass|.web.css|.css|.web.cjs|.cjs)
    at ModuleResolver.resolveDependency (/Users/evanbacon/Documents/GitHub/expo/node_modules/metro/src/node-haste/DependencyGraph/ModuleResolution.js:114:15)
    at DependencyGraph.resolveDependency (/Users/evanbacon/Documents/GitHub/expo/node_modules/metro/src/node-haste/DependencyGraph.js:277:43)
    at /Users/evanbacon/Documents/GitHub/expo/node_modules/metro/src/lib/transformHelpers.js:169:21
    at Server._resolveRelativePath (/Users/evanbacon/Documents/GitHub/expo/node_modules/metro/src/Server.js:1045:12)
    at Server.requestProcessor [as _processSourceMapRequest] (/Users/evanbacon/Documents/GitHub/expo/node_modules/metro/src/Server.js:449:37)
    at Server._processRequest (/Users/evanbacon/Documents/GitHub/expo/node_modules/metro/src/Server.js:396:7)

This is because the HMR sourceMappingURL is being calculated relative to the projectRoot instead of the serverRoot, but all files are being resolved relative to the serverRoot.

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 (a7ae0d4) 83.09%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1137 +/- ## ======================================= Coverage 83.09% 83.09% ======================================= Files 206 206 Lines 10549 10550 +1 Branches 2619 2620 +1 ======================================= + Hits 8766 8767 +1 Misses 1783 1783 ``` | [Files](https://app.codecov.io/gh/facebook/metro/pull/1137?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=facebook) | Coverage Δ | | |---|---|---| | [.../metro/src/DeltaBundler/Serializers/hmrJSBundle.js](https://app.codecov.io/gh/facebook/metro/pull/1137?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=facebook#diff-cGFja2FnZXMvbWV0cm8vc3JjL0RlbHRhQnVuZGxlci9TZXJpYWxpemVycy9obXJKU0J1bmRsZS5qcw==) | `82.92% <100.00%> (+0.42%)` | :arrow_up: |

: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

Apologies for the delay with this, I wanted to try this to make sure I understood how it all fits together - can confirm the root cause is incorrectly generated source (map) URLs, and this LGTM.

Before

image

After

image
robhogan commented 1 year ago

We use unstable_serverRoot heavily in Expo to support monorepos

I'm keen to promote some of these unstable_ features to stable given how load-bearing Expo's use makes them, but tbh I'm not totally sure what the concrete use case is for serverRoot in new projects.

Why not set projectRoot to the workspace root instead?

facebook-github-bot commented 1 year ago

@robhogan merged this pull request in facebook/metro@402dc0b327a0ac9eabf4280b514f9e35254110c7.