From 0df1541e9890bc758ba0c328bfece180e9bc515b Mon Sep 17 00:00:00 2001 From: Rob Hogan Date: Wed, 30 Aug 2023 10:24:08 -0700 Subject: [PATCH] Disable global package resolution by default 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 --- docs/Configuration.md | 6 +-- .../__snapshots__/loadConfig-test.js.snap | 8 +-- packages/metro-config/src/defaults/index.js | 2 +- .../__snapshots__/resolver-test.js.snap | 34 +++++++++---- .../DeltaBundler/__tests__/resolver-test.js | 51 ++++++++++--------- 5 files changed, 58 insertions(+), 43 deletions(-) diff --git a/docs/Configuration.md b/docs/Configuration.md index 8d61fae029..ec37bcc061 100644 --- a/docs/Configuration.md +++ b/docs/Configuration.md @@ -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` diff --git a/packages/metro-config/src/__tests__/__snapshots__/loadConfig-test.js.snap b/packages/metro-config/src/__tests__/__snapshots__/loadConfig-test.js.snap index 4c5e91284e..b329cb4eaf 100644 --- a/packages/metro-config/src/__tests__/__snapshots__/loadConfig-test.js.snap +++ b/packages/metro-config/src/__tests__/__snapshots__/loadConfig-test.js.snap @@ -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 [], @@ -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 [], @@ -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 [], @@ -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 [], diff --git a/packages/metro-config/src/defaults/index.js b/packages/metro-config/src/defaults/index.js index 2e4bc1adeb..7630c51c66 100644 --- a/packages/metro-config/src/defaults/index.js +++ b/packages/metro-config/src/defaults/index.js @@ -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: [], diff --git a/packages/metro/src/DeltaBundler/__tests__/__snapshots__/resolver-test.js.snap b/packages/metro/src/DeltaBundler/__tests__/__snapshots__/resolver-test.js.snap index 0d61bd6eb7..8b12b1c4c1 100644 --- a/packages/metro/src/DeltaBundler/__tests__/__snapshots__/resolver-test.js.snap +++ b/packages/metro/src/DeltaBundler/__tests__/__snapshots__/resolver-test.js.snap @@ -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`] = ` @@ -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`] = ` diff --git a/packages/metro/src/DeltaBundler/__tests__/resolver-test.js b/packages/metro/src/DeltaBundler/__tests__/resolver-test.js index f97af6dd70..1e3df460be 100644 --- a/packages/metro/src/DeltaBundler/__tests__/resolver-test.js +++ b/packages/metro/src/DeltaBundler/__tests__/resolver-test.js @@ -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': '', @@ -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( @@ -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': '', @@ -2050,6 +2049,7 @@ function dep(name: string): TransformResultDependency { __dirname, '../__fixtures__/hasteImpl.js', ), + enableGlobalPackages: true, }, }; }); @@ -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': '', + }, }, }); @@ -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 () => {