Skip to content

Commit

Permalink
Normalize file paths for require context on Windows (#876)
Browse files Browse the repository at this point in the history
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`](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: #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
  • Loading branch information
byCedric authored and facebook-github-bot committed Oct 4, 2022
1 parent 3ab9c00 commit 4bace2a
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 11 deletions.
Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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;"
`;
13 changes: 13 additions & 0 deletions packages/metro/src/lib/__tests__/contextModuleTemplates-test.js
Expand Up @@ -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();
});
});
12 changes: 9 additions & 3 deletions packages/metro/src/lib/contextModuleTemplates.js
Expand Up @@ -10,6 +10,7 @@
*/

import * as path from 'path';
import * as os from 'os';
import type {ContextMode} from '../ModuleGraph/worker/collectDependencies';

function createFileMap(
Expand All @@ -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
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit 4bace2a

Please sign in to comment.