From bd567e5faa5605e553a51d8e0b5a8e7260f805a2 Mon Sep 17 00:00:00 2001 From: Tommy Nguyen <4123478+tido64@users.noreply.github.com> Date: Mon, 17 Apr 2023 12:03:29 +0200 Subject: [PATCH] let the minifier handle comments --- .../__snapshots__/loadConfig-test.js.snap | 8 +- packages/metro-config/src/defaults/index.js | 2 +- .../src/__tests__/index-test.js | 193 +++++++----------- packages/metro-transform-worker/src/index.js | 7 +- .../__snapshots__/buildGraph-test.js.snap | 163 ++++++++++----- 5 files changed, 200 insertions(+), 173 deletions(-) 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 cbe0a51e2d..7fd5637344 100644 --- a/packages/metro-config/src/__tests__/__snapshots__/loadConfig-test.js.snap +++ b/packages/metro-config/src/__tests__/__snapshots__/loadConfig-test.js.snap @@ -132,6 +132,7 @@ Object { }, "output": Object { "ascii_only": true, + "comments": false, "quote_style": 3, "wrap_iife": true, }, @@ -151,7 +152,6 @@ Object { "unstable_dependencyMapReservedName": null, "unstable_disableModuleWrapping": false, "unstable_disableNormalizePseudoGlobals": false, - "unstable_preserveComments": false, "workerPath": "metro/src/DeltaBundler/Worker", }, "transformerPath": "", @@ -311,6 +311,7 @@ Object { }, "output": Object { "ascii_only": true, + "comments": false, "quote_style": 3, "wrap_iife": true, }, @@ -330,7 +331,6 @@ Object { "unstable_dependencyMapReservedName": null, "unstable_disableModuleWrapping": false, "unstable_disableNormalizePseudoGlobals": false, - "unstable_preserveComments": false, "workerPath": "metro/src/DeltaBundler/Worker", }, "transformerPath": "", @@ -490,6 +490,7 @@ Object { }, "output": Object { "ascii_only": true, + "comments": false, "quote_style": 3, "wrap_iife": true, }, @@ -509,7 +510,6 @@ Object { "unstable_dependencyMapReservedName": null, "unstable_disableModuleWrapping": false, "unstable_disableNormalizePseudoGlobals": false, - "unstable_preserveComments": false, "workerPath": "metro/src/DeltaBundler/Worker", }, "transformerPath": "", @@ -669,6 +669,7 @@ Object { }, "output": Object { "ascii_only": true, + "comments": false, "quote_style": 3, "wrap_iife": true, }, @@ -688,7 +689,6 @@ Object { "unstable_dependencyMapReservedName": null, "unstable_disableModuleWrapping": false, "unstable_disableNormalizePseudoGlobals": false, - "unstable_preserveComments": false, "workerPath": "metro/src/DeltaBundler/Worker", }, "transformerPath": "", diff --git a/packages/metro-config/src/defaults/index.js b/packages/metro-config/src/defaults/index.js index d26fb0cb45..c88cb1811a 100644 --- a/packages/metro-config/src/defaults/index.js +++ b/packages/metro-config/src/defaults/index.js @@ -111,6 +111,7 @@ const getDefaultValues = (projectRoot: ?string): ConfigT => ({ }, output: { ascii_only: true, + comments: false, quote_style: 3, wrap_iife: true, }, @@ -134,7 +135,6 @@ const getDefaultValues = (projectRoot: ?string): ConfigT => ({ unstable_disableModuleWrapping: false, unstable_disableNormalizePseudoGlobals: false, unstable_compactOutput: false, - unstable_preserveComments: false, }, watcher: { additionalExts, diff --git a/packages/metro-transform-worker/src/__tests__/index-test.js b/packages/metro-transform-worker/src/__tests__/index-test.js index f94148dced..61399ed10a 100644 --- a/packages/metro-transform-worker/src/__tests__/index-test.js +++ b/packages/metro-transform-worker/src/__tests__/index-test.js @@ -12,10 +12,15 @@ 'use strict'; jest - .mock('../utils/getMinifier', () => () => ({code, map}) => ({ - code: code.replace('arbitrary(code)', 'minified(code)'), - map, - })) + .mock('../utils/getMinifier', () => () => ({code, map, config}) => { + const trimmed = config.output.comments + ? code + : code.replace('/*#__PURE__*/', ''); + return { + code: trimmed.replace('arbitrary(code)', 'minified(code)'), + map, + }; + }) .mock('metro-transform-plugins', () => ({ ...jest.requireActual('metro-transform-plugins'), inlinePlugin: () => ({}), @@ -23,7 +28,7 @@ jest })) .mock('metro-minify-terser'); -import type {JsTransformerConfig} from '../index'; +import type {JsTransformerConfig, JsTransformOptions} from '../index'; import typeof TransformerType from '../index'; import typeof FSType from 'fs'; @@ -54,7 +59,7 @@ const baseConfig: JsTransformerConfig = { enableBabelRuntime: true, globalPrefix: '', hermesParser: false, - minifierConfig: {}, + minifierConfig: {output: {comments: false}}, minifierPath: 'minifyModulePath', optimizationSizeLimit: 100000, publicPath: '/assets', @@ -63,7 +68,17 @@ const baseConfig: JsTransformerConfig = { unstable_disableModuleWrapping: false, unstable_disableNormalizePseudoGlobals: false, unstable_allowRequireContext: false, - unstable_preserveComments: false, +}; + +const baseTransformOptions: JsTransformOptions = { + dev: true, + hot: false, + inlinePlatform: false, + inlineRequires: false, + minify: false, + platform: 'ios', + type: 'module', + unstable_transformProfile: 'default', }; beforeEach(() => { @@ -87,11 +102,7 @@ it('transforms a simple script', async () => { '/root', 'local/file.js', Buffer.from('someReallyArbitrary(code)', 'utf8'), - // $FlowFixMe[prop-missing] Added when annotating Transformer. - { - dev: true, - type: 'script', - }, + {...baseTransformOptions, type: 'script'}, ); expect(result.output[0].type).toBe('js/script'); @@ -113,11 +124,7 @@ it('transforms a simple module', async () => { '/root', 'local/file.js', Buffer.from('arbitrary(code)', 'utf8'), - // $FlowFixMe[prop-missing] Added when annotating Transformer. - { - dev: true, - type: 'module', - }, + baseTransformOptions, ); expect(result.output[0].type).toBe('js/module'); @@ -143,11 +150,7 @@ it('transforms a module with dependencies', async () => { '/root', 'local/file.js', Buffer.from(contents, 'utf8'), - // $FlowFixMe[prop-missing] Added when annotating Transformer. - { - dev: true, - type: 'module', - }, + baseTransformOptions, ); expect(result.output[0].type).toBe('js/module'); @@ -183,11 +186,7 @@ it('transforms an es module with asyncToGenerator', async () => { '/root', 'local/file.js', Buffer.from('export async function test() {}', 'utf8'), - // $FlowFixMe[prop-missing] Added when annotating Transformer. - { - dev: true, - type: 'module', - }, + baseTransformOptions, ); expect(result.output[0].type).toBe('js/module'); @@ -212,11 +211,7 @@ it('transforms async generators', async () => { '/root', 'local/file.js', Buffer.from('export async function* test() { yield "ok"; }', 'utf8'), - // $FlowFixMe[prop-missing] Added when annotating Transformer. - { - dev: true, - type: 'module', - }, + baseTransformOptions, ); expect(result.output[0].data.code).toMatchSnapshot(); @@ -244,12 +239,7 @@ it('transforms import/export syntax when experimental flag is on', async () => { '/root', 'local/file.js', Buffer.from(contents, 'utf8'), - // $FlowFixMe[prop-missing] Added when annotating Transformer. - { - dev: true, - experimentalImportSupport: true, - type: 'module', - }, + {...baseTransformOptions, experimentalImportSupport: true}, ); expect(result.output[0].type).toBe('js/module'); @@ -280,12 +270,7 @@ it('does not add "use strict" on non-modules', async () => { '/root', 'node_modules/local/file.js', Buffer.from('module.exports = {};', 'utf8'), - // $FlowFixMe[prop-missing] Added when annotating Transformer. - { - dev: true, - experimentalImportSupport: true, - type: 'module', - }, + {...baseTransformOptions, experimentalImportSupport: true}, ); expect(result.output[0].type).toBe('js/module'); @@ -305,11 +290,7 @@ it('preserves require() calls when module wrapping is disabled', async () => { '/root', 'local/file.js', Buffer.from(contents, 'utf8'), - // $FlowFixMe[prop-missing] Added when annotating Transformer. - { - dev: true, - type: 'module', - }, + baseTransformOptions, ); expect(result.output[0].type).toBe('js/module'); @@ -329,11 +310,7 @@ it('reports filename when encountering unsupported dynamic dependency', async () '/root', 'local/file.js', Buffer.from(contents, 'utf8'), - // $FlowFixMe[prop-missing] Added when annotating Transformer. - { - dev: true, - type: 'module', - }, + baseTransformOptions, ); throw new Error('should not reach this'); } catch (error) { @@ -352,11 +329,7 @@ it('supports dynamic dependencies from within `node_modules`', async () => { '/root', 'node_modules/foo/bar.js', Buffer.from('require(foo.bar);', 'utf8'), - // $FlowFixMe[prop-missing] Added when annotating Transformer. - { - dev: true, - type: 'module', - }, + baseTransformOptions, ) ).output[0].data.code, ).toBe( @@ -378,12 +351,7 @@ it('minifies the code correctly', async () => { '/root', 'local/file.js', Buffer.from('arbitrary(code);', 'utf8'), - // $FlowFixMe[prop-missing] Added when annotating Transformer. - { - dev: true, - minify: true, - type: 'module', - }, + {...baseTransformOptions, minify: true}, ) ).output[0].data.code, ).toBe([HEADER_PROD, ' minified(code);', '});'].join('\n')); @@ -397,12 +365,7 @@ it('minifies a JSON file', async () => { '/root', 'local/file.json', Buffer.from('arbitrary(code);', 'utf8'), - // $FlowFixMe[prop-missing] Added when annotating Transformer. - { - dev: true, - minify: true, - type: 'module', - }, + {...baseTransformOptions, minify: true}, ) ).output[0].data.code, ).toBe( @@ -425,11 +388,7 @@ it('does not wrap a JSON file when disableModuleWrapping is enabled', async () = '/root', 'local/file.json', Buffer.from('arbitrary(code);', 'utf8'), - // $FlowFixMe[prop-missing] Added when annotating Transformer. - { - dev: true, - type: 'module', - }, + baseTransformOptions, ) ).output[0].data.code, ).toBe('module.exports = arbitrary(code);;'); @@ -441,12 +400,7 @@ it('uses a reserved dependency map name and prevents it from being minified', as '/root', 'local/file.js', Buffer.from('arbitrary(code);', 'utf8'), - // $FlowFixMe[prop-missing] Added when annotating Transformer. - { - dev: false, - minify: true, - type: 'module', - }, + {...baseTransformOptions, dev: false, minify: true}, ); expect(result.output[0].data.code).toMatchInlineSnapshot(` "__d(function (g, r, i, a, m, e, THE_DEP_MAP) { @@ -465,12 +419,7 @@ it('throws if the reserved dependency map name appears in the input', async () = 'arbitrary(code); /* the code is not allowed to mention THE_DEP_MAP, even in a comment */', 'utf8', ), - // $FlowFixMe[prop-missing] Added when annotating Transformer. - { - dev: false, - minify: true, - type: 'module', - }, + {...baseTransformOptions, dev: false, minify: true}, ), ).rejects.toThrowErrorMatchingInlineSnapshot( `"Source code contains the reserved string \`THE_DEP_MAP\` at character offset 55"`, @@ -483,12 +432,7 @@ it('allows disabling the normalizePseudoGlobals pass when minifying', async () = '/root', 'local/file.js', Buffer.from('arbitrary(code);', 'utf8'), - // $FlowFixMe[prop-missing] Added when annotating Transformer. - { - dev: false, - minify: true, - type: 'module', - }, + {...baseTransformOptions, dev: false, minify: true}, ); expect(result.output[0].data.code).toMatchInlineSnapshot(` "__d(function (global, _$$_REQUIRE, _$$_IMPORT_DEFAULT, _$$_IMPORT_ALL, module, exports, _dependencyMap) { @@ -503,12 +447,7 @@ it('allows emitting compact code when not minifying', async () => { '/root', 'local/file.js', Buffer.from('arbitrary(code);', 'utf8'), - // $FlowFixMe[prop-missing] Added when annotating Transformer. - { - dev: false, - minify: false, - type: 'module', - }, + {...baseTransformOptions, dev: false, minify: false}, ); expect(result.output[0].data.code).toMatchInlineSnapshot( `"__d(function(global,_$$_REQUIRE,_$$_IMPORT_DEFAULT,_$$_IMPORT_ALL,module,exports,_dependencyMap){arbitrary(code);});"`, @@ -521,11 +460,10 @@ it('skips minification in Hermes stable transform profile', async () => { '/root', 'local/file.js', Buffer.from('arbitrary(code);', 'utf8'), - // $FlowFixMe[prop-missing] Added when annotating Transformer. { + ...baseTransformOptions, dev: false, minify: true, - type: 'module', unstable_transformProfile: 'hermes-canary', }, ); @@ -542,11 +480,10 @@ it('skips minification in Hermes canary transform profile', async () => { '/root', 'local/file.js', Buffer.from('arbitrary(code);', 'utf8'), - // $FlowFixMe[prop-missing] Added when annotating Transformer. { + ...baseTransformOptions, dev: false, minify: true, - type: 'module', unstable_transformProfile: 'hermes-canary', }, ); @@ -564,12 +501,7 @@ it('counts all line endings correctly', async () => { '/root', 'local/file.js', Buffer.from(str, 'utf8'), - // $FlowFixMe[prop-missing] Added when annotating Transformer. - { - dev: false, - minify: false, - type: 'module', - }, + {...baseTransformOptions, dev: false, minify: false}, ); const differentEndingsResult = await transformStr( @@ -585,18 +517,13 @@ it('counts all line endings correctly', async () => { ); }); -it('allows preservation of comments', async () => { +it('outputs comments when `minify: false`', async () => { const result = await Transformer.transform( - {...baseConfig, unstable_preserveComments: true}, + baseConfig, '/root', 'local/file.js', Buffer.from('/*#__PURE__*/arbitrary(code);', 'utf8'), - // $FlowFixMe[prop-missing] Added when annotating Transformer. - { - dev: false, - minify: false, - type: 'module', - }, + {...baseTransformOptions, dev: false, minify: false}, ); expect(result.output[0].data.code).toMatchInlineSnapshot(` "__d(function (global, _$$_REQUIRE, _$$_IMPORT_DEFAULT, _$$_IMPORT_ALL, module, exports, _dependencyMap) { @@ -604,3 +531,33 @@ it('allows preservation of comments', async () => { });" `); }); + +it('omits comments when `minify: true`', async () => { + const result = await Transformer.transform( + baseConfig, + '/root', + 'local/file.js', + Buffer.from('/*#__PURE__*/arbitrary(code);', 'utf8'), + {...baseTransformOptions, dev: false, minify: true}, + ); + expect(result.output[0].data.code).toMatchInlineSnapshot(` + "__d(function (g, r, i, a, m, e, d) { + minified(code); + });" + `); +}); + +it('allows outputting comments when `minify: true`', async () => { + const result = await Transformer.transform( + {...baseConfig, minifierConfig: {output: {comments: true}}}, + '/root', + 'local/file.js', + Buffer.from('/*#__PURE__*/arbitrary(code);', 'utf8'), + {...baseTransformOptions, dev: false, minify: true}, + ); + expect(result.output[0].data.code).toMatchInlineSnapshot(` + "__d(function (g, r, i, a, m, e, d) { + /*#__PURE__*/minified(code); + });" + `); +}); diff --git a/packages/metro-transform-worker/src/index.js b/packages/metro-transform-worker/src/index.js index 5a32b15947..f369411576 100644 --- a/packages/metro-transform-worker/src/index.js +++ b/packages/metro-transform-worker/src/index.js @@ -96,7 +96,6 @@ export type JsTransformerConfig = $ReadOnly<{ unstable_compactOutput: boolean, /** Enable `require.context` statements which can be used to import multiple files in a directory. */ unstable_allowRequireContext: boolean, - unstable_preserveComments: boolean, }>; export type {CustomTransformOptions} from 'metro-babel-transformer'; @@ -300,7 +299,7 @@ async function transformJS( babelrc: false, code: false, configFile: false, - comments: config.unstable_preserveComments, + comments: true, filename: file.filename, plugins, sourceMaps: false, @@ -323,7 +322,7 @@ async function transformJS( babelrc: false, code: false, configFile: false, - comments: config.unstable_preserveComments, + comments: true, filename: file.filename, plugins: [ [metroTransformPlugins.constantFoldingPlugin, babelPluginOpts], @@ -408,7 +407,7 @@ async function transformJS( const result = generate( wrappedAst, { - comments: config.unstable_preserveComments, + comments: true, compact: config.unstable_compactOutput, filename: file.filename, retainLines: false, diff --git a/packages/metro/src/integration_tests/__tests__/__snapshots__/buildGraph-test.js.snap b/packages/metro/src/integration_tests/__tests__/__snapshots__/buildGraph-test.js.snap index 40e783d11a..386f55e8e6 100644 --- a/packages/metro/src/integration_tests/__tests__/__snapshots__/buildGraph-test.js.snap +++ b/packages/metro/src/integration_tests/__tests__/__snapshots__/buildGraph-test.js.snap @@ -5,10 +5,21 @@ Array [ Object { "data": Object { "code": "__d(function (global, _$$_REQUIRE, _$$_IMPORT_DEFAULT, _$$_IMPORT_ALL, module, exports, _dependencyMap) { + /** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @format + * + */ + 'use strict'; var Bar = _$$_REQUIRE(_dependencyMap[0]); var Foo = _$$_REQUIRE(_dependencyMap[1]); + // $FlowFixMe: Flow doesn't understand TypeScript var TypeScript = _$$_REQUIRE(_dependencyMap[2]); Object.keys(Object.assign({}, Bar)); module.exports = { @@ -23,297 +34,357 @@ Array [ "", ], }, - "lineCount": 13, + "lineCount": 24, "map": Array [ Array [ 2, 2, - 11, + 1, + 0, + ], + Array [ + 3, + 0, + 2, + 0, + ], + Array [ + 4, + 0, + 3, + 0, + ], + Array [ + 5, + 0, + 4, + 0, + ], + Array [ + 6, + 0, + 5, + 0, + ], + Array [ + 7, + 0, + 6, + 0, + ], + Array [ + 8, + 0, + 7, + 0, + ], + Array [ + 9, + 0, + 8, 0, ], Array [ + 10, + 0, + 9, + 0, + ], + Array [ + 12, 2, + 11, + 0, + ], + Array [ + 12, 14, 11, 12, ], Array [ - 4, + 14, 2, 13, 0, ], Array [ - 4, + 14, 6, 13, 6, "Bar", ], Array [ - 4, + 14, 9, 13, 9, ], Array [ - 4, + 14, 12, 13, 12, "require", ], Array [ - 4, + 14, 23, 13, 19, ], Array [ - 4, + 14, 42, 13, 28, ], Array [ - 5, + 15, 2, 14, 0, ], Array [ - 5, + 15, 6, 14, 6, "Foo", ], Array [ - 5, + 15, 9, 14, 9, ], Array [ - 5, + 15, 12, 14, 12, "require", ], Array [ - 5, + 15, 23, 14, 19, ], Array [ - 5, + 15, 42, 14, 28, ], Array [ - 6, + 16, + 2, + 15, + 0, + ], + Array [ + 17, 2, 16, 0, ], Array [ - 6, + 17, 6, 16, 6, "TypeScript", ], Array [ - 6, + 17, 16, 16, 16, ], Array [ - 6, + 17, 19, 16, 19, "require", ], Array [ - 6, + 17, 30, 16, 26, ], Array [ - 6, + 17, 49, 16, 42, ], Array [ - 7, + 18, 2, 18, 0, "Object", ], Array [ - 7, + 18, 8, 18, 6, ], Array [ - 7, + 18, 9, 18, 7, "keys", ], Array [ - 7, + 18, 13, 18, 11, ], Array [ - 7, + 18, 32, 18, 16, "Bar", ], Array [ - 7, + 18, 35, 18, 19, ], Array [ - 7, + 18, 37, 18, 21, ], Array [ - 8, + 19, 2, 20, 0, "module", ], Array [ - 8, + 19, 8, 20, 6, ], Array [ - 8, + 19, 9, 20, 7, "exports", ], Array [ - 8, + 19, 16, 20, 14, ], Array [ - 8, + 19, 19, 20, 17, ], Array [ - 9, + 20, 4, 20, 18, "Foo", ], Array [ - 9, + 20, 7, 20, 21, ], Array [ - 9, + 20, 9, 20, 18, "Foo", ], Array [ - 9, + 20, 12, 20, 21, ], Array [ - 10, + 21, 4, 20, 23, "Bar", ], Array [ - 10, + 21, 7, 20, 26, ], Array [ - 10, + 21, 9, 20, 23, "Bar", ], Array [ - 10, + 21, 12, 20, 26, ], Array [ - 11, + 22, 4, 20, 28, "TypeScript", ], Array [ - 11, + 22, 14, 20, 38, ], Array [ - 11, + 22, 16, 20, 28, "TypeScript", ], Array [ - 12, + 23, 2, 20, 38, ], Array [ - 12, + 23, 3, 20, 39, ], Array [ - 13, + 24, 0, 20, 40,