From 4bace2a9f32c55d901d87cb77850fefcc81881f9 Mon Sep 17 00:00:00 2001 From: Cedric van Putten Date: Tue, 4 Oct 2022 08:04:58 -0700 Subject: [PATCH] Normalize file paths for require context on Windows (#876) 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 https://github.com/expo/router/issues/13. When inspecting the [`contextModuleTemplates`](https://github.com/facebook/metro/blob/main/packages/metro/src/lib/contextModuleTemplates.js#L44-L46), using [this example repo](https://github.com/byCedric/expo-router-context-windows-issue), you can see this happening. ![image](https://user-images.githubusercontent.com/1203991/192898803-f5030e8f-b9b3-4b88-8f2d-636f713d7e89.png) 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](https://github.com/facebook/metro/blob/main/packages/metro/src/Assets.js#L201-L204)) Pull Request resolved: https://github.com/facebook/metro/pull/876 Test Plan: - Added a windows style test case, copied from [this test in `metro-file-map`](https://github.com/facebook/metro/blob/main/packages/metro-file-map/src/lib/__tests__/normalizePathSep-test.js#L21-L26) - Ran the tests on my local Windows machine ![image](https://user-images.githubusercontent.com/1203991/192899575-371aec94-032b-46c8-b884-cd1e1c44f1b1.png) - Patched the [test repo](https://github.com/byCedric/expo-router-context-windows-issue) with [this change](https://github.com/byCedric/expo-router-context-windows-issue/blob/main/patches/metro%2B0.72.3.patch) and validated the output on Windows ![image](https://user-images.githubusercontent.com/1203991/192900417-bd38c02c-d8c9-4598-8bc0-b39fbbdcb3c7.png) Reviewed By: huntie Differential Revision: D39923633 Pulled By: huntie fbshipit-source-id: c80eed410df0a4270e2d00d5b0ff0aca6e29ff0b --- .../contextModuleTemplates-test.js.snap | 39 +++++++++++++++---- .../__tests__/contextModuleTemplates-test.js | 13 +++++++ .../metro/src/lib/contextModuleTemplates.js | 12 ++++-- 3 files changed, 53 insertions(+), 11 deletions(-) diff --git a/packages/metro/src/lib/__tests__/__snapshots__/contextModuleTemplates-test.js.snap b/packages/metro/src/lib/__tests__/__snapshots__/contextModuleTemplates-test.js.snap index 1926b05ca2..5505bd6347 100644 --- a/packages/metro/src/lib/__tests__/__snapshots__/contextModuleTemplates-test.js.snap +++ b/packages/metro/src/lib/__tests__/__snapshots__/contextModuleTemplates-test.js.snap @@ -5,11 +5,11 @@ exports[`getContextModuleTemplate creates a lazy template 1`] = ` const map = Object.defineProperties({}, { \\"./foo.js\\": { enumerable: true, get() { return import(\\"/path/to/project/src/foo.js\\"); } }, }); - + function metroContext(request) { return map[request]; } - + // Return the keys that can be resolved. metroContext.keys = function metroContextKeys() { return Object.keys(map); @@ -29,11 +29,11 @@ const map = Object.defineProperties({}, { \\"./another/bar.js\\": { enumerable: true, get() { return import(\\"/path/to/project/src/another/bar.js\\"); } }, \\"./foo.js\\": { enumerable: true, get() { return import(\\"/path/to/project/src/foo.js\\"); } }, }); - + function metroContext(request) { return map[request]; } - + // Return the keys that can be resolved. metroContext.keys = function metroContextKeys() { return Object.keys(map); @@ -52,11 +52,11 @@ exports[`getContextModuleTemplate creates a sync template 1`] = ` const map = Object.defineProperties({}, { \\"./foo.js\\": { enumerable: true, get() { return require(\\"/path/to/project/src/foo.js\\"); } }, }); - + function metroContext(request) { return map[request]; } - + // Return the keys that can be resolved. metroContext.keys = function metroContextKeys() { return Object.keys(map); @@ -75,13 +75,13 @@ exports[`getContextModuleTemplate creates an eager template 1`] = ` const map = Object.defineProperties({}, { \\"./foo.js\\": { enumerable: true, get() { return require(\\"/path/to/project/src/foo.js\\"); } }, }); - + function metroContext(request) { // Here Promise.resolve().then() is used instead of new Promise() to prevent // uncaught exception popping up in devtools return Promise.resolve().then(() => map[request]); } - + // Return the keys that can be resolved. metroContext.keys = function metroContextKeys() { return Object.keys(map); @@ -113,3 +113,26 @@ metroEmptyContext.resolve = function metroContextResolve(request) { module.exports = metroEmptyContext;" `; + +exports[`getContextModuleTemplate creates posix paths on windows for sync template 1`] = ` +"// All of the requested modules are loaded behind enumerable getters. +const map = Object.defineProperties({}, { + \\"./foo.js\\": { enumerable: true, get() { return require(\\"C:\\\\\\\\path\\\\\\\\to\\\\\\\\project\\\\\\\\src\\\\\\\\foo.js\\"); } }, +}); + +function metroContext(request) { + return map[request]; +} + +// Return the keys that can be resolved. +metroContext.keys = function metroContextKeys() { + return Object.keys(map); +}; + +// Return the module identifier for a user request. +metroContext.resolve = function metroContextResolve(request) { + throw new Error('Unimplemented Metro module context functionality'); +} + +module.exports = metroContext;" +`; diff --git a/packages/metro/src/lib/__tests__/contextModuleTemplates-test.js b/packages/metro/src/lib/__tests__/contextModuleTemplates-test.js index 0b9e86892d..d869f05826 100644 --- a/packages/metro/src/lib/__tests__/contextModuleTemplates-test.js +++ b/packages/metro/src/lib/__tests__/contextModuleTemplates-test.js @@ -49,4 +49,17 @@ describe('getContextModuleTemplate', () => { expect(template).toMatchSnapshot(); }); + + test('creates posix paths on windows for sync template', () => { + jest.resetModules(); + jest.mock('path', () => jest.requireActual('path').win32); + const { + getContextModuleTemplate: getWindowsTemplate, + } = require('../contextModuleTemplates'); + const template = getWindowsTemplate('sync', 'c:/path/to/project/src', [ + 'C:\\path\\to\\project\\src\\foo.js', + ]); + expect(template).toMatch(/foo\.js/); + expect(template).toMatchSnapshot(); + }); }); diff --git a/packages/metro/src/lib/contextModuleTemplates.js b/packages/metro/src/lib/contextModuleTemplates.js index ac07c17048..e1e27a6ed5 100644 --- a/packages/metro/src/lib/contextModuleTemplates.js +++ b/packages/metro/src/lib/contextModuleTemplates.js @@ -10,6 +10,7 @@ */ import * as path from 'path'; +import * as os from 'os'; import type {ContextMode} from '../ModuleGraph/worker/collectDependencies'; function createFileMap( @@ -26,14 +27,19 @@ function createFileMap( .forEach(file => { let filePath = path.relative(modulePath, file); + if (os.platform() === 'win32') { + filePath = filePath.replace(/\\/g, '/'); + } + // NOTE(EvanBacon): I'd prefer we prevent the ability for a module to require itself (`require.context('./')`) // but Webpack allows this, keeping it here provides better parity between bundlers. // Ensure relative file paths start with `./` so they match the // patterns (filters) used to include them. if (!filePath.startsWith('.')) { - filePath = `.${path.sep}` + filePath; + filePath = `./${filePath}`; } + const key = JSON.stringify(filePath); // NOTE(EvanBacon): Webpack uses `require.resolve` in order to load modules on demand, // Metro doesn't have this functionality so it will use getters instead. Modules need to @@ -79,11 +85,11 @@ const map = ${createFileMap( files, moduleId => `${importSyntax}(${JSON.stringify(moduleId)})`, )}; - + function metroContext(request) { ${getContextTemplate} } - + // Return the keys that can be resolved. metroContext.keys = function metroContextKeys() { return Object.keys(map);