From bada7f8684b4ee2325b4138615f66f451502deab Mon Sep 17 00:00:00 2001 From: Jean Lauliac Date: Mon, 30 Oct 2017 10:50:40 -0700 Subject: [PATCH] metro-bundler: ModuleGraph tests: @flow Summary: Having types in tests ensure that we're testing what the API is supposed to process and not something else. In that case, adding type revealed that we were mistakenly passing strings instead of `Buffer` objects to the transform and optimize functions, but it would still work because `toString` was called over it. Passing proper `Buffer` objects as expected ensures that it doesn't just work with strings, and that the integration of these files in the rest of the application works as expected. This changeset fixes these callsites and add a few invariants here and there. We use `invariant` instead of `expect()` because Flow understands only the former as a type refinement, even though `expect` would also be a form of possible refinement. I don't think it's a big deal. Reviewed By: davidaurelio Differential Revision: D6160019 fbshipit-source-id: cbcbb05d7bccf9e1b9f6bb3ea30b0bb2925aef1b --- .../src/ModuleGraph/types.flow.js | 2 +- .../__tests__/collect-dependencies-test.js | 3 +- .../worker/__tests__/optimize-module-test.js | 51 ++++++++--- .../worker/__tests__/transform-module-test.js | 91 ++++++++++++------- .../worker/__tests__/wrap-worker-fn-test.js | 3 +- 5 files changed, 101 insertions(+), 49 deletions(-) diff --git a/packages/metro-bundler/src/ModuleGraph/types.flow.js b/packages/metro-bundler/src/ModuleGraph/types.flow.js index ca57984b..041e3c29 100644 --- a/packages/metro-bundler/src/ModuleGraph/types.flow.js +++ b/packages/metro-bundler/src/ModuleGraph/types.flow.js @@ -150,7 +150,7 @@ export type TransformResult = {| export type TransformResults = {[string]: TransformResult}; -export type TransformVariants = {+[name: string]: {}, +default: {}}; +export type TransformVariants = {+[name: string]: {}}; export type TransformedCodeFile = {| +file: string, diff --git a/packages/metro-bundler/src/ModuleGraph/worker/__tests__/collect-dependencies-test.js b/packages/metro-bundler/src/ModuleGraph/worker/__tests__/collect-dependencies-test.js index 53191a0d..b0bf1c50 100644 --- a/packages/metro-bundler/src/ModuleGraph/worker/__tests__/collect-dependencies-test.js +++ b/packages/metro-bundler/src/ModuleGraph/worker/__tests__/collect-dependencies-test.js @@ -6,8 +6,9 @@ * LICENSE file in the root directory of this source tree. An additional grant * of patent rights can be found in the PATENTS file in the same directory. * - * @format * @emails oncall+javascript_foundation + * @flow + * @format */ 'use strict'; diff --git a/packages/metro-bundler/src/ModuleGraph/worker/__tests__/optimize-module-test.js b/packages/metro-bundler/src/ModuleGraph/worker/__tests__/optimize-module-test.js index 40305106..65b606a9 100644 --- a/packages/metro-bundler/src/ModuleGraph/worker/__tests__/optimize-module-test.js +++ b/packages/metro-bundler/src/ModuleGraph/worker/__tests__/optimize-module-test.js @@ -6,17 +6,21 @@ * LICENSE file in the root directory of this source tree. An additional grant * of patent rights can be found in the PATENTS file in the same directory. * - * @format * @emails oncall+javascript_foundation + * @flow + * @format */ 'use strict'; +const invariant = require('fbjs/lib/invariant'); +const nullthrows = require('fbjs/lib/nullthrows'); const optimizeModule = require('../optimize-module'); const transformModule = require('../transform-module'); const transformer = require('../../../transformer.js'); -const {SourceMapConsumer} = require('source-map'); + const {fn} = require('../../test-helpers'); +const {SourceMapConsumer} = require('source-map'); const {objectContaining} = jasmine; @@ -27,21 +31,27 @@ describe('optimizing JS modules', () => { platform: 'android', postMinifyProcess: x => x, }; - const originalCode = `if (Platform.OS !== 'android') { + const originalCode = new Buffer( + `if (Platform.OS !== 'android') { require('arbitrary-dev'); } else { __DEV__ ? require('arbitrary-android-dev') : require('arbitrary-android-prod'); - }`; + }`, + 'utf8', + ); let transformResult; beforeAll(() => { const result = transformModule(originalCode, {filename, transformer}); - transformResult = JSON.stringify({type: 'code', details: result.details}); + transformResult = new Buffer( + JSON.stringify({type: 'code', details: result.details}), + 'utf8', + ); }); it('copies everything from the transformed file, except for transform results', () => { const result = optimizeModule(transformResult, optimizationOptions); - const expected = JSON.parse(transformResult).details; + const expected = JSON.parse(transformResult.toString('utf8')).details; delete expected.transformed; expect(result.type).toBe('code'); expect(result.details).toEqual(objectContaining(expected)); @@ -51,14 +61,19 @@ describe('optimizing JS modules', () => { let dependencyMapName, injectedVars, optimized, requireName; beforeAll(() => { const result = optimizeModule(transformResult, optimizationOptions); + invariant(result.type === 'code', 'result must be code'); optimized = result.details.transformed.default; - injectedVars = optimized.code.match(/function\(([^)]*)/)[1].split(','); + injectedVars = nullthrows( + optimized.code.match(/function\(([^)]*)/), + )[1].split(','); [, requireName, , , dependencyMapName] = injectedVars; }); it('optimizes code', () => { expect(optimized.code).toEqual( - `__d(function(${injectedVars}){${requireName}(${dependencyMapName}[0])});`, + `__d(function(${injectedVars.join( + ',', + )}){${requireName}(${dependencyMapName}[0])});`, ); }); @@ -69,7 +84,9 @@ describe('optimizing JS modules', () => { it('creates source maps', () => { const consumer = new SourceMapConsumer(optimized.map); const column = optimized.code.lastIndexOf(requireName + '('); - const loc = findLast(originalCode, 'require'); + const loc = nullthrows( + findLast(originalCode.toString('utf8'), 'require'), + ); expect(consumer.originalPositionFor({line: 1, column})).toEqual( objectContaining(loc), @@ -80,8 +97,9 @@ describe('optimizing JS modules', () => { const result = optimizeModule(transformResult, { ...optimizationOptions, isPolyfill: true, - }).details; - expect(result.transformed.default.dependencies).toEqual([]); + }); + invariant(result.type === 'code', 'result must be code'); + expect(result.details.transformed.default.dependencies).toEqual([]); }); }); @@ -99,6 +117,7 @@ describe('optimizing JS modules', () => { it('passes the result to the provided postprocessing function', () => { postMinifyProcess.stub.callsFake(x => x); const result = optimize(); + invariant(result.type === 'code', 'result must be code'); const {code, map} = result.details.transformed.default; expect(postMinifyProcess).toBeCalledWith({code, map}); }); @@ -107,7 +126,9 @@ describe('optimizing JS modules', () => { const code = 'var postprocessed = "code";'; const map = {version: 3, mappings: 'postprocessed'}; postMinifyProcess.stub.returns({code, map}); - expect(optimize().details.transformed.default).toEqual( + const result = optimize(); + invariant(result.type === 'code', 'result must be code'); + expect(result.details.transformed.default).toEqual( objectContaining({code, map}), ); }); @@ -116,7 +137,11 @@ describe('optimizing JS modules', () => { it('passes through non-code data unmodified', () => { const data = {type: 'asset', details: {arbitrary: 'data'}}; expect( - optimizeModule(JSON.stringify(data), {dev: true, platform: ''}), + optimizeModule(new Buffer(JSON.stringify(data), 'utf8'), { + dev: true, + platform: '', + postMinifyProcess: ({code, map}) => ({code, map}), + }), ).toEqual(data); }); }); diff --git a/packages/metro-bundler/src/ModuleGraph/worker/__tests__/transform-module-test.js b/packages/metro-bundler/src/ModuleGraph/worker/__tests__/transform-module-test.js index 0480436d..ec2f480a 100644 --- a/packages/metro-bundler/src/ModuleGraph/worker/__tests__/transform-module-test.js +++ b/packages/metro-bundler/src/ModuleGraph/worker/__tests__/transform-module-test.js @@ -6,21 +6,26 @@ * LICENSE file in the root directory of this source tree. An additional grant * of patent rights can be found in the PATENTS file in the same directory. * - * @format * @emails oncall+javascript_foundation + * @flow + * @format */ 'use strict'; +const invariant = require('fbjs/lib/invariant'); +const nullthrows = require('fbjs/lib/nullthrows'); +const t = require('babel-types'); const transformModule = require('../transform-module'); -const t = require('babel-types'); -const {SourceMapConsumer} = require('source-map'); const {fn} = require('../../test-helpers'); const {parse} = require('babylon'); +const {SourceMapConsumer} = require('source-map'); const generate = require('babel-generator').default; const {traverse} = require('babel-core'); +import type {TransformVariants} from '../../types.flow'; + jest.mock('image-size', () => buffer => { return JSON.parse(buffer.toString('utf8')).__size; }); @@ -33,13 +38,14 @@ describe('transforming JS modules:', () => { beforeEach(() => { transformer = { transform: fn(), + getCacheKey: () => 'foo', }; transformer.transform.stub.returns(transformResult()); }); const {bodyAst, sourceCode, transformedCode} = createTestData(); - const options = variants => ({ + const options = (variants?: TransformVariants) => ({ filename, transformer, variants, @@ -61,7 +67,7 @@ describe('transforming JS modules:', () => { it('exposes a haste ID if present', () => { const hasteID = 'TheModule'; - const codeWithHasteID = `/** @providesModule ${hasteID} */`; + const codeWithHasteID = toBuffer(`/** @providesModule ${hasteID} */`); const result = transformModule(codeWithHasteID, options()); expect(result.type).toBe('code'); expect(result.details).toEqual(expect.objectContaining({hasteID})); @@ -88,27 +94,23 @@ describe('transforming JS modules:', () => { projectRoot: '', }; - it( - 'calls the passed-in transform function with code, file name, and options ' + - 'for all passed in variants', - () => { - const variants = {dev: {dev: true}, prod: {dev: false}}; + it('calls the passed-in transform function with code, file name, and options for all passed in variants', () => { + const variants = {dev: {dev: true}, prod: {dev: false}}; - transformModule(sourceCode, options(variants)); - expect(transformer.transform).toBeCalledWith({ - filename, - localPath: filename, - options: {...defaults, ...variants.dev}, - src: sourceCode, - }); - expect(transformer.transform).toBeCalledWith({ - filename, - localPath: filename, - options: {...defaults, ...variants.prod}, - src: sourceCode, - }); - }, - ); + transformModule(sourceCode, options(variants)); + expect(transformer.transform).toBeCalledWith({ + filename, + localPath: filename, + options: {...defaults, ...variants.dev}, + src: sourceCode.toString('utf8'), + }); + expect(transformer.transform).toBeCalledWith({ + filename, + localPath: filename, + options: {...defaults, ...variants.prod}, + src: sourceCode.toString('utf8'), + }); + }); it('calls back with any error yielded by the transform function', () => { const error = new Error(); @@ -124,7 +126,9 @@ describe('transforming JS modules:', () => { it('wraps the code produced by the transform function into a module factory', () => { const result = transformModule(sourceCode, options()); + invariant(result.type === 'code', 'result must be code'); const {code, dependencyMapName} = result.details.transformed.default; + invariant(dependencyMapName != null, 'dependencyMapName cannot be null'); expect(code.replace(/\s+/g, '')).toEqual( `__d(function(global,require,module,exports,${dependencyMapName}){${transformedCode}});`, ); @@ -132,6 +136,7 @@ describe('transforming JS modules:', () => { it('wraps the code produced by the transform function into an IIFE for polyfills', () => { const result = transformModule(sourceCode, {...options(), polyfill: true}); + invariant(result.type === 'code', 'result must be code'); const {code} = result.details.transformed.default; expect(code.replace(/\s+/g, '')).toEqual( `(function(global){${transformedCode}})(this);`, @@ -140,6 +145,7 @@ describe('transforming JS modules:', () => { it('creates source maps', () => { const result = transformModule(sourceCode, options()); + invariant(result.type === 'code', 'result must be code'); const {code, map} = result.details.transformed.default; const position = findColumnAndLine(code, 'code'); expect(position).not.toBeNull(); @@ -157,7 +163,8 @@ describe('transforming JS modules:', () => { const {body} = parse(code).program; transformer.transform.stub.returns(transformResult(body)); - const result = transformModule(code, options()); + const result = transformModule(toBuffer(code), options()); + invariant(result.type === 'code', 'result must be code'); expect(result.details.transformed.default).toEqual( expect.objectContaining({dependencies: [dep1, dep2]}), ); @@ -172,19 +179,28 @@ describe('transforming JS modules:', () => { .returns(transformResult([])); const result = transformModule(sourceCode, options(variants)); + invariant(result.type === 'code', 'result must be code'); const {dev, prod} = result.details.transformed; expect(dev.code.replace(/\s+/g, '')).toEqual( - `__d(function(global,require,module,exports,${dev.dependencyMapName}){arbitrary(code);});`, + `__d(function(global,require,module,exports,${nullthrows( + dev.dependencyMapName, + )}){arbitrary(code);});`, ); expect(prod.code.replace(/\s+/g, '')).toEqual( - `__d(function(global,require,module,exports,${prod.dependencyMapName}){arbitrary(code);});`, + `__d(function(global,require,module,exports,${nullthrows( + prod.dependencyMapName, + )}){arbitrary(code);});`, ); }); it('prefixes JSON files with `module.exports = `', () => { const json = '{"foo":"bar"}'; - const result = transformModule(json, {...options(), filename: 'some.json'}); + const result = transformModule(toBuffer(json), { + ...options(), + filename: 'some.json', + }); + invariant(result.type === 'code', 'result must be code'); const {code} = result.details.transformed.default; expect(code.replace(/\s+/g, '')).toEqual( '__d(function(global,require,module,exports){' + @@ -193,7 +209,11 @@ describe('transforming JS modules:', () => { }); it('does not create source maps for JSON files', () => { - const result = transformModule('{}', {...options(), filename: 'some.json'}); + const result = transformModule(toBuffer('{}'), { + ...options(), + filename: 'some.json', + }); + invariant(result.type === 'code', 'result must be code'); expect(result.details.transformed.default).toEqual( expect.objectContaining({map: null}), ); @@ -207,10 +227,11 @@ describe('transforming JS modules:', () => { 'react-native': {'react-native': 'defs'}, }; - const result = transformModule(JSON.stringify(pkg), { + const result = transformModule(toBuffer(JSON.stringify(pkg)), { ...options(), filename: 'arbitrary/package.json', }); + invariant(result.type === 'code', 'result must be code'); expect(result.details.package).toEqual(pkg); }); @@ -219,7 +240,7 @@ describe('transforming JS modules:', () => { const image = {__size: {width: 30, height: 20}}; ['foo.png', 'foo@2x.ios.png'].forEach(filePath => { expect( - transformModule(new Buffer(JSON.stringify(image), 'utf8'), { + transformModule(toBuffer(JSON.stringify(image)), { ...options(), filename: filePath, }), @@ -249,7 +270,7 @@ function createTestData() { }); return { bodyAst: fileAst.program.body, - sourceCode, + sourceCode: toBuffer(sourceCode), transformedCode: generate(fileAst).code, }; } @@ -265,3 +286,7 @@ function findColumnAndLine(text, string) { } return null; } + +function toBuffer(str) { + return new Buffer(str, 'utf8'); +} diff --git a/packages/metro-bundler/src/ModuleGraph/worker/__tests__/wrap-worker-fn-test.js b/packages/metro-bundler/src/ModuleGraph/worker/__tests__/wrap-worker-fn-test.js index a0329ce3..ebb10ec9 100644 --- a/packages/metro-bundler/src/ModuleGraph/worker/__tests__/wrap-worker-fn-test.js +++ b/packages/metro-bundler/src/ModuleGraph/worker/__tests__/wrap-worker-fn-test.js @@ -6,8 +6,9 @@ * LICENSE file in the root directory of this source tree. An additional grant * of patent rights can be found in the PATENTS file in the same directory. * - * @format * @emails oncall+javascript_foundation + * @flow + * @format */ 'use strict';