-
Notifications
You must be signed in to change notification settings - Fork 639
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
Normalize file paths for require context on Windows #876
Normalize file paths for require context on Windows #876
Conversation
93cbefc
to
42d200b
Compare
@dmytrorykun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, looks good some little suggestions.
@@ -34,6 +34,10 @@ function createFileMap( | |||
if (!filePath.startsWith('.')) { | |||
filePath = `.${path.sep}` + filePath; | |||
} | |||
// NOTE(byCedric): On Windows, normalize the backslashes to forward slashes to match the behavior in Webpack. | |||
if (path.sep === '\\') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets use os.platform
here
if (path.sep === '\\') { | |
if (os.platform === 'win32') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also by using os.platform
you can remove the comment as code itself is self explanatory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: If we move this logic above the preceding if
statement, it's slightly cleaner to have normalised filePath
first, before operating on it (e.g. adding relative path prefix).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done and done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Metro have a standard way of mocking os.platform()
to be win32
? I'd prefer to add a test that runs in "windows mode" because I don't think there is a windows CI runner on Metro atm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not AFAIK, but can be achieved with Jest mocks. I think the added unit test makes sense — it's an intentional and direct test against the fork in logic implemented. IMO (weakly held, see below) it's nice that the base tests deal without the concern of POSIX/Windows paths except where we expect specific handling.
At the same time, we would harden test coverage if all tests ran for both path types, however this will not be solved immediately via a separate win32
test runner — as the input strings to all tests would need to be changed appropriately (and at which point could be handled on a Unix host). I guess we could explore some abstractions, possibly:
- Simple:
describe.each()
(inside an abstraction) across suitable sets of tests for "posix paths" and "win32 paths" which mock relevantos
andpath
methods in thewin32
case, and the use ofpath.normalize()
for every input string in these tests (and snapshot/path.sep
-aware assertions).- What we get: All test cases are run at development time. Tests and snapshot tests are cleanly grouped by path behaviour (where use of
path.normalize()
alone will cause snapshot mismatches across platforms).
- What we get: All test cases are run at development time. Tests and snapshot tests are cleanly grouped by path behaviour (where use of
- Fancy: Setting up "posix paths" and "win32 paths" globally for certain tests matching an extension pattern (e.g.
.multiplatform.test.js
, akin to the multiplatform Jest setup ofjest-expo/universal
— with output like this). In this case, we might opt to label these "unix" and "win32 (simulated)" with a wider set of mocked differences.- Improvements: More transparent behaviour when opted-in via test filename (no extra
describe
wrapper, snapshots automatically suffixed). Fancy CLI output for test cases (platform tag). Single source of truth for handling in test setup scripts. When actually running on Windows, would run only "win32" test suite (and dropping "(simulated)" suffix in CLI output).
- Improvements: More transparent behaviour when opted-in via test filename (no extra
With these ideas listed out, you might agree that our existing case-by-case handling is simple and adequate 😅.
Of course, this is not the only functional difference we may encounter on Windows, so I'm in favour of adding a win32
CI test job as a baseline. cc @motiz88
packages/metro/src/lib/__tests__/contextModuleTemplates-test.js
Outdated
Show resolved
Hide resolved
9641708
to
9f80195
Compare
@byCedric could you please rebase your branch a the latest main |
9f80195
to
2131e76
Compare
Done! @dmytrorykun 😄 |
e52b21b
to
5fa03ad
Compare
Done again! @dmytrorykun 😄 |
@dmytrorykun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@byCedric could you please rebase your branch on the latest main once again |
5fa03ad
to
04a8509
Compare
Done @dmytrorykun, but can we please get this merged before the next change? 😄 |
Co-authored-by: Alex Hunt <hello@alexhunt.io>
04a8509
to
17fc817
Compare
@dmytrorykun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
…rsions Summary: Enables Windows test runs in CircleCI. Partially resolves this discussion: facebook#876 (comment). - Adds `test-windows` job via the `circleci/windows` orb (inspired by the main react-native repo CircleCI config: https://fburl.com/hxyx4han). - `test-linux` job continues to use fast `cimg/node` images. - Parametarises `node-version` on the `test-linux` and `test-windows` jobs, and assigns as a matrix of versions (remains a single run against `14.17.0` for now). - Drops the `yarn_run` command in favour of YAML anchor to explicitly mix in `&secure_unset_publish_token` behaviour using CircleCI's `environment` option, which is cross platform (see test plan). NOTE: Since Windows tests are failing en masse, the branch filter `only: /windows\/.*/` is added, effectively disabling this job except on `windows/`-prefixed branches (which will provide a hook for fixing these in future). ``` Test Suites: 18 failed, 92 passed, 110 total Tests: 232 failed, 1 skipped, 1286 passed, 1519 total Snapshots: 20 failed, 403 passed, 423 total Time: 87.927 s Ran all test suites. ``` Changelog: [Internal] Differential Revision: D40805193 fbshipit-source-id: a86689f6fc864483c12677e573df5c287aa63181
…rsions (#885) Summary: Pull Request resolved: #885 Enables Windows test runs in CircleCI. Partially resolves this discussion: #876 (comment). - Adds `test-windows` job via the `circleci/windows` orb (inspired by the main react-native repo [CircleCI config](https://github.com/facebook/react-native/blob/ad5e3f6b9ae870cbfcef2874511915c5dc309ce8/.circleci/config.yml#L960-L962)). - `test-linux` job continues to use fast `cimg/node` images. - Parametarises `node-version` on the `test-linux` and `test-windows` jobs, and assigns as a matrix of versions (remains a single run against `14.17` for now). - Drops the `yarn_run` command in favour of YAML anchor to explicitly mix in `&secure_unset_publish_token` behaviour using CircleCI's `environment` option, which is cross platform (see test plan). NOTE: Since Windows tests are failing en masse, the branch filter `only: /windows\/.*/` is added, effectively disabling this job except on `windows/`-prefixed branches (which will provide a hook for fixing these in future). ``` Test Suites: 18 failed, 92 passed, 110 total Tests: 232 failed, 1 skipped, 1286 passed, 1519 total Snapshots: 20 failed, 403 passed, 423 total Time: 87.927 s Ran all test suites. ``` https://app.circleci.com/pipelines/github/huntie/metro/9/workflows/2d5be9e2-40bf-4dc1-a450-621656f3cfc4/jobs/25 Changelog: [Internal] Reviewed By: jacdebug Differential Revision: D40805193 fbshipit-source-id: 011e1937c909deaa013bb6257a6a3df181f31e86
….0 (#264) [](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [metro-react-native-babel-preset](https://togithub.com/facebook/metro) | [`^0.72.0` -> `^0.76.0`](https://renovatebot.com/diffs/npm/metro-react-native-babel-preset/0.72.3/0.76.0) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>facebook/metro</summary> ### [`v0.76.0`](https://togithub.com/facebook/metro/releases/tag/v0.76.0) [Compare Source](https://togithub.com/facebook/metro/compare/v0.75.1...v0.76.0) - **\[Breaking]**: Increase minimum Node version from 14 to 16. (facebook/metro@e5950ae by [@​huntie](https://togithub.com/huntie)) - **\[Breaking]**: Remove `isAssetFile` from custom resolver context, add `assetExts`. (facebook/metro@c6548f7 by [@​huntie](https://togithub.com/huntie)) - **\[Feature]**: Support [`require.resolveWeak()`](https://facebook.github.io/metro/docs/module-api#requireresolveweak). (facebook/metro@354d6e4 by [@​motiz88](https://togithub.com/motiz88)) - **\[Fix]**: Don't over-invalidate on symlink changes if resolution through symlinks is not enabled. (facebook/metro@2303c10 by [@​robhogan](https://togithub.com/robhogan)) - **\[Fix]**: Returning `false` from [`context.redirectModulePath`](https://facebook.github.io/metro/docs/resolution#redirectmodulepath-string--string--false) will resolve to empty module in all cases. (facebook/metro@0f1846a by [@​huntie](https://togithub.com/huntie)) - **\[Fix]**: Respect extensionless entries in `browser`, `react-native` etc when resolving subpath package specifiers. (facebook/metro@7e92227 by [@​huntie](https://togithub.com/huntie)) - **\[Fix]**: Remove undocumented Meta-only `__jsResource` and `__conditionallySplitJsResource` functions from module API. (facebook/metro@f1d905b and facebook/metro@69c8fc7 by [@​motiz88](https://togithub.com/motiz88)) > NOTE: Experimental features are not covered by semver and can change at any time. - **\[Experimental]**: Fixes and improvements for symlink support. (facebook/metro@0e2a70a, facebook/metro@3bef954, and facebook/metro@eeb211f by [@​robhogan](https://togithub.com/robhogan)) - **\[Experimental]**: Fix bug where `"exports"` field would be used on relative imports within a package. (facebook/metro@cd25c2b by [@​huntie](https://togithub.com/huntie)) ### [`v0.75.1`](https://togithub.com/facebook/metro/releases/tag/v0.75.1) [Compare Source](https://togithub.com/facebook/metro/compare/v0.75.0...v0.75.1) - **\[Feature]**: `metro-inspector-proxy`: Add a human-readable reference to each inspector entries/pages.([https://github.com/facebook/metro/pull/921](https://togithub.com/facebook/metro/pull/921) by [@​byCedric](https://togithub.com/byCedric)) - **\[Feature]**: `metro-inspector-proxy`: Report errors in the console. (facebook/metro@da8b41b by [@​mattbfb](https://togithub.com/mattbfb)) - **\[Fix]**: Race condition where a very recently modified file might have missing metadata.(facebook/metro@baf28ab by [@​robhogan](https://togithub.com/robhogan)) - **\[Fix]**: Source maps may have invalid entries when using Terser minification. ([https://github.com/facebook/metro/pull/928](https://togithub.com/facebook/metro/pull/928) by [@​robhogan](https://togithub.com/robhogan)) - **\[Fix]**: `metro-inspector-proxy`: Fetch source maps from Metro. (facebook/metro@6690b39 by [@​mattbfb](https://togithub.com/mattbfb)) - **\[Fix]**: Mitigate potential source map mismatches with concurrent transformations due to [terser#​1341](https://togithub.com/terser/terser/issues/1341). ([https://github.com/facebook/metro/pull/929](https://togithub.com/facebook/metro/pull/929) by [@​robhogan](https://togithub.com/robhogan)) > NOTE: Experimental features are not covered by semver and can change at any time. - **\[Experimental]**: Add initial_build annotation to Resolving and Transforming Dependencies (facebook/metro@fc83b52 by [@​blakef](https://togithub.com/blakef)) - **\[Experimental]**: Implement support for Package Exports (enabled via `resolver.unstable_enablePackageExports`) (facebook/metro@4d7ab38, facebook/metro@38b96f8, facebook/metro@216d3e2, facebook/metro@6e6f36f by [@​huntie](https://togithub.com/huntie)) - **\[Experimental]**: Implement support for symlinks (enabled via `resolver.unstable_enableSymlinks`) ([https://github.com/facebook/metro/pull/925](https://togithub.com/facebook/metro/pull/925), [https://github.com/facebook/metro/pull/926](https://togithub.com/facebook/metro/pull/926), etc. by [@​robhogan](https://togithub.com/robhogan)) **Full Changelog:** facebook/metro@v0.75.0...v0.75.1 ### [`v0.75.0`](https://togithub.com/facebook/metro/releases/tag/v0.75.0) [Compare Source](https://togithub.com/facebook/metro/compare/v0.74.1...v0.75.0) - **\[Breaking]**: Formalise minimum Node JS requirement at 14.17.0 via `package.json#engines`. (facebook/metro@c3e453e) - **\[Breaking]**: Filter untyped context properties passed to custom resolvers. (facebook/metro@cb01ec0) - **\[Breaking]**: Change default `context.redirectModulePath` implementation to return absolute path in all cases. (facebook/metro@acbfe63) - **\[Feature]**: Add `mainFields`, `getPackage`, and `getPackageForModule` to custom resolver context. (facebook/metro@adfb593) **Full Changelog**: facebook/metro@v0.74.1...v0.75.0 ### [`v0.74.1`](https://togithub.com/facebook/metro/releases/tag/v0.74.1) [Compare Source](https://togithub.com/facebook/metro/compare/v0.74.0...v0.74.1) - **\[Feature]**: Add `@babel/plugin-proposal-numeric-separator` to `metro-react-native-babel-preset` ([https://github.com/facebook/metro/pull/681](https://togithub.com/facebook/metro/pull/681) by [@​SConaway](https://togithub.com/SConaway)) **Full Changelog**: facebook/metro@v0.74.0...v0.74.1 ### [`v0.74.0`](https://togithub.com/facebook/metro/releases/tag/v0.74.0) [Compare Source](https://togithub.com/facebook/metro/compare/v0.73.8...v0.74.0) - **\[Breaking]** Remove [@​babel/plugin-transform-template-literals](https://togithub.com/babel/plugin-transform-template-literals) from metro-react-native-babel-preset (facebook/metro@322dea8) - **\[Breaking]** Remove `postProcessBundleSourcemap` from config (facebook/metro@339794e) - **\[Fix]** Don't log ENOENT errors to console for expected URL stack frames (facebook/metro@1031ae6) - **\[Fix]** Don't attempt to use the `find` crawler on Windows (facebook/metro@735aa9f) - **\[Performance]** Improve AST processing during transformation ([https://github.com/facebook/metro/pull/854](https://togithub.com/facebook/metro/pull/854) by [@​EvanBacon](https://togithub.com/EvanBacon)) - **\[Performance]** Improve Fast Refresh responsiveness when watching a large number of files (facebook/metro@b942eca) **Full Changelog:** facebook/metro@v0.73.6...v0.74.0 ### [`v0.73.8`](https://togithub.com/facebook/metro/releases/tag/v0.73.8) [Compare Source](https://togithub.com/facebook/metro/compare/v0.73.7...v0.73.8) *This is a hotfix on the `0.73.x` branch.* - **\[Fix]**: Source maps may have invalid entries when using Terser minification. ([https://github.com/facebook/metro/pull/928](https://togithub.com/facebook/metro/pull/928)) - **\[Fix]**: Mitigate potential source map mismatches with concurrent transformations due to [terser#​1341](https://togithub.com/terser/terser/issues/1341). ([https://github.com/facebook/metro/pull/929](https://togithub.com/facebook/metro/pull/929)) **Full Changelog**: facebook/metro@v0.73.7...v0.73.8 ### [`v0.73.7`](https://togithub.com/facebook/metro/releases/tag/v0.73.7) [Compare Source](https://togithub.com/facebook/metro/compare/v0.73.6...v0.73.7) *This is a hotfix on the `0.73.x` branch.* - **\[Fix]** Don't attempt to use the `find` crawler on Windows (facebook/metro@3703019) ### [`v0.73.6`](https://togithub.com/facebook/metro/releases/tag/v0.73.6) [Compare Source](https://togithub.com/facebook/metro/compare/v0.73.5...v0.73.6) - **\[Fix]** Fix duplicate 'add' events, reduce dropped events on new subtrees in `NodeWatcher` (non-Watchman, non-macOS).(facebook/metro@51fb7e3) > NOTE: Experimental features are not covered by semver and can change at any time. - **\[Experimental]** `experimentalImportBundleSupport`: Move bundle path hints into serialised dependency map([https://github.com/facebook/metro/pull/901](https://togithub.com/facebook/metro/pull/901)) **Full Changelog:** facebook/metro@v0.73.5...v0.73.6 ### [`v0.73.5`](https://togithub.com/facebook/metro/releases/tag/v0.73.5) [Compare Source](https://togithub.com/facebook/metro/compare/v0.73.4...v0.73.5) - **\[Fix]**: Make all `getTransformOptions` result properties optional. (facebook/metro@a07c823) - **\[Fix]**: Bug that can lead to "unknown module" errors at runtime after an incremental build. (facebook/metro@b1be263) - **\[Fix]**: `metro-runtime`: Re-throw cached module errors without wrapping. (facebook/metro@032c4a1) - **\[Fix]** Bump `babel/types` dependency to `^7.20.0` for more consistent exposed AST Bump `babel/types` dependency to `^7.20.0` for more consistent exposed AST **Full Changelog**: facebook/metro@v0.73.4...v0.73.5 ### [`v0.73.4`](https://togithub.com/facebook/metro/releases/tag/v0.73.4) [Compare Source](https://togithub.com/facebook/metro/compare/v0.73.3...v0.73.4) - **\[Feature]:** Expose `watch` option in `RunServerOptions` ([https://github.com/facebook/metro/pull/889](https://togithub.com/facebook/metro/pull/889) by [@​EvanBacon](https://togithub.com/EvanBacon)) - **\[Feature]:** `metro-runtime`: Emit additional context on WebSocket `'close'` events (facebook/metro@d54986c) > NOTE: Experimental features are not covered by semver and can change at any time. - **\[Experimental]**: `experimentalImportBundleSupport`: Retraverse parents of deleted async dependencies (facebook/metro@cb806d1) **Full Changelog:** facebook/metro@v0.73.3...v0.73.4 ### [`v0.73.3`](https://togithub.com/facebook/metro/releases/tag/v0.73.3) [Compare Source](https://togithub.com/facebook/metro/compare/v0.73.2...v0.73.3) - **\[Feature]**: Add configurable watcher health check that is off by default (facebook/metro@7adf468, facebook/metro@39f6e50) > NOTE: Experimental features are not covered by semver and can change at any time. - **\[Experimental]**: Move `experimentalImportBundleSupport` option from transformer to server (facebook/metro@3c0e1f7) **Full Changelog:** facebook/metro@v0.73.2...v0.73.3 ### [`v0.73.2`](https://togithub.com/facebook/metro/releases/tag/v0.73.2) [Compare Source](https://togithub.com/facebook/metro/compare/v0.73.1...v0.73.2) Maintenance release with purely internal changes. **Full Changelog:** facebook/metro@v0.73.1...v0.73.2 ### [`v0.73.1`](https://togithub.com/facebook/metro/releases/tag/v0.73.1) [Compare Source](https://togithub.com/facebook/metro/compare/v0.73.0...v0.73.1) - **\[Fix]**: Generate a unique name for each Watchman subscription. ([`3b0e78a`](https://togithub.com/facebook/metro/commit/3b0e78a76f4eea9f02e8b8464cf5b5e4549d6ac7)) > NOTE: Experimental features are not covered by semver and can change at any time. - **\[Experimental]\[Fix]**: Normalize file paths for `require.context` on Windows ([https://github.com/facebook/metro/pull/876](https://togithub.com/facebook/metro/pull/876) by [@​byCedric](https://togithub.com/byCedric)) **Full Changelog:** facebook/metro@v0.73.0...v0.73.1 ### [`v0.73.0`](https://togithub.com/facebook/metro/releases/tag/v0.73.0) [Compare Source](https://togithub.com/facebook/metro/compare/v0.72.3...v0.73.0) - **\[Breaking]** Switch default minifier from `uglify-es` to `terser`. ([#​871](https://togithub.com/facebook/metro/issues/871)) - **\[Breaking]**: Increase minimum supported Node.js version to ^14.17.0. ([#​872](https://togithub.com/facebook/metro/issues/872)) - **\[Breaking]**: Drop support for old (pre-CalVer) Watchman versions. ([`422055a`](https://togithub.com/facebook/metro/commit/422055a5ccaca41edb1864ca07d4f810b3e03791)) - **\[Feature]**: Support `fsevents` watcher on Apple Silicon. ([#​875](https://togithub.com/facebook/metro/issues/875)) - **\[Feature]**: Support loading source URLs in inspector-proxy. ([`db19b06`](https://togithub.com/facebook/metro/commit/db19b06bdd6d2fbbe109e4f3be4b3af3489c1f1c)) - **\[Fix]**: Log warning on unexpected error during `metro-file-map` cache read. ([`7028b7f`](https://togithub.com/facebook/metro/commit/7028b7f51074f9ceef22258a8643d0f90de2388b)) - **\[Fix]**: Remove exponentiation operator transform from `metro-react-native-babel-preset`. ([`c2365bb`](https://togithub.com/facebook/metro/commit/c2365bb1d72a3773b31c05feab13a96afac484df)) - **\[Fix]**: Don’t check `watchman --version` if `useWatchman` is false. ([`76c9307`](https://togithub.com/facebook/metro/commit/76c9307ed61efa7794b30b4e585cc5941ed73e16)) **Full Changelog:** facebook/metro@v0.72.3...v0.73.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/dooboolab/dooboo-ui). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xNjAuMCIsInVwZGF0ZWRJblZlciI6IjM0LjE2MC4wIn0=--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Summary
On Windows, the
require.context
resolves modules to non-posix paths. This seems to diverge from the expected behavior compared to Webpack, where the modules are resolved as posix paths. Because of this, we are running into unfortunate issues like expo/router#13.When inspecting the
contextModuleTemplates
, using this example repo, you can see this happening.This change normalizes the file path keys to be posix, matching the default Webpack behavior. It does not change the absolute file reference. (based on this piece of code)
Test plan
metro-file-map