Skip to content

Commit

Permalink
Disable global package resolution by default
Browse files Browse the repository at this point in the history
Summary:
Prior to Metro symlink support, many users relied on "global package" resolution in setups such as Yarn workspaces. With global packages, any `package.json` *outside* `node_modules` may be resolved by name from anywhere in the project.

In the case of Yarn (etc.) workspaces, we're now able to resolve the same packages through the symlinks the packager creates under `node_modules`, in the same manner as Node JS and other tooling does.

Global packages now provide little value for most users, and in fact are a potential footgun - if your global package happens to share its name with a `node_modules` package, the former will always take precedence, even when resolving relative to another `node_modules` package.

This changes the default for `resolver.enableGlobalPackages` to `false`. If you require global packages, you will need to explicitly enable them in your config.

Changelog
```
 - **[Breaking]:** Disable global package resolution (`resolver.enableGlobalPackages`) by default
```

Reviewed By: motiz88

Differential Revision: D48777893

fbshipit-source-id: 23b8a5884582e20ca92c8356e67909f2d66b9e7b
  • Loading branch information
robhogan authored and facebook-github-bot committed Aug 30, 2023
1 parent 9e7cec1 commit 0df1541
Show file tree
Hide file tree
Showing 5 changed files with 58 additions and 43 deletions.
6 changes: 1 addition & 5 deletions docs/Configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -247,11 +247,7 @@ What module to use as the canonical "empty" module when one is needed. Defaults
Type: `boolean`.
Whether to automatically resolve references first-party packages (e.g. workspaces) in your project. Any `package.json` file with a valid `name` property within `projectRoot` or `watchFolders` (but outside of `node_modules`) counts as a package for this purpose. Defaults to `true`.
:::note
The default value of this option may change in a future version of Metro. If your project relies on it, it's recommended that you set it explicitly in your config file.
:::
Whether to automatically resolve references to first-party packages (e.g. workspaces) in your project. Any `package.json` file with a valid `name` property within `projectRoot` or `watchFolders` (but outside of `node_modules`) counts as a package for this purpose. Defaults to `false`.
#### `extraNodeModules`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ Object {
"dependencyExtractor": undefined,
"disableHierarchicalLookup": false,
"emptyModulePath": "metro-runtime/src/modules/empty-module",
"enableGlobalPackages": true,
"enableGlobalPackages": false,
"extraNodeModules": Object {},
"hasteImplModulePath": undefined,
"nodeModulesPaths": Array [],
Expand Down Expand Up @@ -227,7 +227,7 @@ Object {
"dependencyExtractor": undefined,
"disableHierarchicalLookup": false,
"emptyModulePath": "metro-runtime/src/modules/empty-module",
"enableGlobalPackages": true,
"enableGlobalPackages": false,
"extraNodeModules": Object {},
"hasteImplModulePath": undefined,
"nodeModulesPaths": Array [],
Expand Down Expand Up @@ -405,7 +405,7 @@ Object {
"dependencyExtractor": undefined,
"disableHierarchicalLookup": false,
"emptyModulePath": "metro-runtime/src/modules/empty-module",
"enableGlobalPackages": true,
"enableGlobalPackages": false,
"extraNodeModules": Object {},
"hasteImplModulePath": undefined,
"nodeModulesPaths": Array [],
Expand Down Expand Up @@ -583,7 +583,7 @@ Object {
"dependencyExtractor": undefined,
"disableHierarchicalLookup": false,
"emptyModulePath": "metro-runtime/src/modules/empty-module",
"enableGlobalPackages": true,
"enableGlobalPackages": false,
"extraNodeModules": Object {},
"hasteImplModulePath": undefined,
"nodeModulesPaths": Array [],
Expand Down
2 changes: 1 addition & 1 deletion packages/metro-config/src/defaults/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ const getDefaultValues = (projectRoot: ?string): ConfigT => ({
emptyModulePath: require.resolve(
'metro-runtime/src/modules/empty-module.js',
),
enableGlobalPackages: true,
enableGlobalPackages: false,
extraNodeModules: {},
hasteImplModulePath: undefined,
nodeModulesPaths: [],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,19 @@ None of these files exist:
3 | import bar from 'foo';"
`;

exports[`linux global packages default config uses the name in the package.json as the package name 1`] = `
exports[`linux global packages default config does not resolve global packages 1`] = `
"Unable to resolve module aPackage from /root/index.js: aPackage could not be found within the project.
1 | import foo from 'bar';
> 2 | import a from 'aPackage';
| ^
3 | import bar from 'foo';"
> 1 |"
`;

exports[`linux global packages default config does not resolve global packages 2`] = `
"Unable to resolve module aPackage/ from /root/index.js: aPackage/ could not be found within the project.
> 1 |"
`;

exports[`linux global packages default config does not resolve global packages 3`] = `
"Unable to resolve module aPackage/other from /root/index.js: aPackage/other could not be found within the project.
> 1 |"
`;

exports[`linux global packages explicitly disabled does not resolve global packages 1`] = `
Expand Down Expand Up @@ -248,12 +255,19 @@ None of these files exist:
3 | import bar from 'foo';"
`;

exports[`win32 global packages default config uses the name in the package.json as the package name 1`] = `
exports[`win32 global packages default config does not resolve global packages 1`] = `
"Unable to resolve module aPackage from C:\\\\root\\\\index.js: aPackage could not be found within the project.
1 | import foo from 'bar';
> 2 | import a from 'aPackage';
| ^
3 | import bar from 'foo';"
> 1 |"
`;

exports[`win32 global packages default config does not resolve global packages 2`] = `
"Unable to resolve module aPackage/ from C:\\\\root\\\\index.js: aPackage/ could not be found within the project.
> 1 |"
`;

exports[`win32 global packages default config does not resolve global packages 3`] = `
"Unable to resolve module aPackage/other from C:\\\\root\\\\index.js: aPackage/other could not be found within the project.
> 1 |"
`;

exports[`win32 global packages explicitly disabled does not resolve global packages 1`] = `
Expand Down
51 changes: 28 additions & 23 deletions packages/metro/src/DeltaBundler/__tests__/resolver-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1633,17 +1633,12 @@ function dep(name: string): TransformResultDependency {
});

describe('global packages', () => {
describe.each([
{name: 'default config', config: {}},
{
name: 'explicitly enabled',
config: {
resolver: {
enableGlobalPackages: true,
},
describe('explicitly enabled', () => {
const config = {
resolver: {
enableGlobalPackages: true,
},
},
])('$name', ({config}) => {
};
it('treats any folder with a package.json as a global package', async () => {
setMockFileSystem({
'index.js': '',
Expand Down Expand Up @@ -1843,7 +1838,7 @@ function dep(name: string): TransformResultDependency {
},
});

await expect(createResolver()).rejects.toThrow(
await expect(createResolver(config)).rejects.toThrow(
'Duplicated files or mocks. Please check the console for more info',
);
expect(console.error).toHaveBeenCalledWith(
Expand Down Expand Up @@ -1989,13 +1984,17 @@ function dep(name: string): TransformResultDependency {
});
});

describe('explicitly disabled', () => {
const config = {
resolver: {
enableGlobalPackages: false,
describe.each([
{name: 'default config', config: {}},
{
name: 'explicitly disabled',
config: {
resolver: {
enableGlobalPackages: false,
},
},
};

},
])('$name', ({config}) => {
test('does not resolve global packages', async () => {
setMockFileSystem({
'index.js': '',
Expand Down Expand Up @@ -2050,6 +2049,7 @@ function dep(name: string): TransformResultDependency {
__dirname,
'../__fixtures__/hasteImpl.js',
),
enableGlobalPackages: true,
},
};
});
Expand Down Expand Up @@ -2381,10 +2381,12 @@ function dep(name: string): TransformResultDependency {
'package.json': '{}',
'index.js': '',
},
foo: {
'package.json': JSON.stringify({name: 'foo'}),
lib: {'bar.js': ''},
'index.js': '',
node_modules: {
foo: {
'package.json': JSON.stringify({name: 'foo'}),
lib: {'bar.js': ''},
'index.js': '',
},
},
});

Expand All @@ -2396,11 +2398,14 @@ function dep(name: string): TransformResultDependency {
resolver.resolve(p('/root/folder/index.js'), dep('foo')),
).toEqual({
type: 'sourceFile',
filePath: p('/root/foo/index.js'),
filePath: p('/root/node_modules/foo/index.js'),
});
expect(
resolver.resolve(p('/root/folder/index.js'), dep('foo/lib/bar')),
).toEqual({type: 'sourceFile', filePath: p('/root/foo/lib/bar.js')});
).toEqual({
type: 'sourceFile',
filePath: p('/root/node_modules/foo/lib/bar.js'),
});
});

it('supports scoped `extraNodeModules`', async () => {
Expand Down

0 comments on commit 0df1541

Please sign in to comment.