From 2b5b729bbbee8fa84264c1ee229ce85777c3f3f6 Mon Sep 17 00:00:00 2001 From: Jean Lauliac Date: Fri, 12 May 2017 10:26:46 -0700 Subject: [PATCH] packager: remove getPlatformExtension default platforms Summary: Default arguments are dangerous, and it shows here again. `Server` was calling that function without passing down the platforms, meaning custom platform would not be accounted for, possibly causing all kinds of bugs for OSS use cases. Additional, the typing of `platform` across the stack was wrong: it can be `null`, in which case we resolve the files without extension. The reason Flow didn't detect that issue before is because we use `Object.assign` to re-export the function from `DependencyGraph`, but Flow is not smart enough to propagate the types in that particular case. I'll remove all the other re-export, as it may uncover further type errors. Reviewed By: davidaurelio Differential Revision: D5052070 fbshipit-source-id: 7b427ec036ca74b5dcd7c88f7dfa0df541b8eb6b --- packager/src/Bundler/index.js | 8 +++--- packager/src/JSTransformer/worker/inline.js | 2 +- packager/src/JSTransformer/worker/worker.js | 6 ++--- packager/src/Resolver/index.js | 2 +- packager/src/Server/__tests__/Server-test.js | 6 ++--- packager/src/Server/index.js | 10 ++++--- packager/src/lib/GlobalTransformCache.js | 3 +-- packager/src/node-haste/DependencyGraph.js | 2 -- .../__tests__/getAssetDataFromName-test.js | 26 ++++++++++--------- .../__tests__/getPlatformExtension-test.js | 21 +++++++-------- .../node-haste/lib/getPlatformExtension.js | 16 ++++-------- 11 files changed, 48 insertions(+), 54 deletions(-) diff --git a/packager/src/Bundler/index.js b/packager/src/Bundler/index.js index 43142909c..58531819b 100644 --- a/packager/src/Bundler/index.js +++ b/packager/src/Bundler/index.js @@ -62,7 +62,7 @@ export type ExtraTransformOptions = { export type GetTransformOptionsOpts = {| dev: boolean, hot: boolean, - platform: string, + platform: ?string, |}; export type GetTransformOptions = ( @@ -521,7 +521,7 @@ class Bundler { generateSourceMaps = false, }: { entryFile: string, - platform: string, + platform: ?string, dev?: boolean, minify?: boolean, hot?: boolean, @@ -560,7 +560,7 @@ class Bundler { onProgress, }: { entryFile: string, - platform: string, + platform: ?string, dev?: boolean, minify?: boolean, hot?: boolean, @@ -784,7 +784,7 @@ class Bundler { generateSourceMaps: boolean, hot: boolean, minify: boolean, - platform: string, + platform: ?string, projectRoots: Array, |}, ): Promise { diff --git a/packager/src/JSTransformer/worker/inline.js b/packager/src/JSTransformer/worker/inline.js index 6094a7cb6..041c903ff 100644 --- a/packager/src/JSTransformer/worker/inline.js +++ b/packager/src/JSTransformer/worker/inline.js @@ -170,7 +170,7 @@ type AstResult = { function inline( filename: string, transformResult: {ast?: ?Ast, code: string, map: ?MappingsMap}, - options: {+dev: boolean, +platform: string}, + options: {+dev: boolean, +platform: ?string}, ): AstResult { const code = transformResult.code; const babelOptions = { diff --git a/packager/src/JSTransformer/worker/worker.js b/packager/src/JSTransformer/worker/worker.js index c7452e06b..65e3b8922 100644 --- a/packager/src/JSTransformer/worker/worker.js +++ b/packager/src/JSTransformer/worker/worker.js @@ -47,7 +47,7 @@ export type TransformOptionsStrict = {| +generateSourceMaps: boolean, +hot: boolean, +inlineRequires: {+blacklist: {[string]: true}} | boolean, - +platform: string, + +platform: ?string, +projectRoot: string, |}; @@ -56,14 +56,14 @@ export type TransformOptions = { +generateSourceMaps?: boolean, +hot?: boolean, +inlineRequires?: {+blacklist: {[string]: true}} | boolean, - +platform: string, + +platform: ?string, +projectRoot: string, }; export type Options = {| +dev: boolean, +minify: boolean, - +platform: string, + +platform: ?string, +transform: TransformOptionsStrict, |}; diff --git a/packager/src/Resolver/index.js b/packager/src/Resolver/index.js index 77c8f0b8b..476ef3ae5 100644 --- a/packager/src/Resolver/index.js +++ b/packager/src/Resolver/index.js @@ -98,7 +98,7 @@ class Resolver { getDependencies( entryPath: string, - options: {platform: string, recursive?: boolean}, + options: {platform: ?string, recursive?: boolean}, bundlingOptions: T, onProgress?: ?(finishedModules: number, totalModules: number) => mixed, getModuleId: mixed, diff --git a/packager/src/Server/__tests__/Server-test.js b/packager/src/Server/__tests__/Server-test.js index 2dfa6d3d1..01e8ec46e 100644 --- a/packager/src/Server/__tests__/Server-test.js +++ b/packager/src/Server/__tests__/Server-test.js @@ -155,7 +155,7 @@ describe('processRequest', () => { isolateModuleIDs: false, minify: false, onProgress: jasmine.any(Function), - platform: undefined, + platform: null, resolutionResponse: null, runBeforeMainModule: ['InitializeCore'], runModule: true, @@ -209,7 +209,7 @@ describe('processRequest', () => { isolateModuleIDs: false, minify: false, onProgress: jasmine.any(Function), - platform: undefined, + platform: null, resolutionResponse: null, runBeforeMainModule: ['InitializeCore'], runModule: true, @@ -457,7 +457,7 @@ describe('processRequest', () => { isolateModuleIDs: false, minify: false, onProgress: null, - platform: undefined, + platform: null, resolutionResponse: null, runBeforeMainModule: ['InitializeCore'], runModule: false, diff --git a/packager/src/Server/index.js b/packager/src/Server/index.js index 0e0f53717..8bd3f8917 100644 --- a/packager/src/Server/index.js +++ b/packager/src/Server/index.js @@ -12,11 +12,11 @@ 'use strict'; const AssetServer = require('../AssetServer'); -const getPlatformExtension = require('../node-haste/DependencyGraph').getPlatformExtension; const Bundler = require('../Bundler'); const MultipartResponse = require('./MultipartResponse'); const defaults = require('../../defaults'); +const getPlatformExtension = require('../node-haste/lib/getPlatformExtension'); const mime = require('mime-types'); const path = require('path'); const symbolicate = require('./symbolicate'); @@ -148,6 +148,7 @@ class Server { _hmrFileChangeListener: ?(type: string, filePath: string) => mixed; _reporter: Reporter; _symbolicateInWorker: Symbolicate; + _platforms: Set; constructor(options: Options) { this._opts = { @@ -181,6 +182,7 @@ class Server { this._bundles = Object.create(null); this._changeWatchers = []; this._fileChangeListeners = []; + this._platforms = new Set(this._opts.platforms); this._assetServer = new AssetServer({ assetExts: this._opts.assetExts, @@ -281,7 +283,7 @@ class Server { getShallowDependencies(options: DependencyOptions): Promise> { return Promise.resolve().then(() => { const platform = options.platform != null - ? options.platform : getPlatformExtension(options.entryFile); + ? options.platform : getPlatformExtension(options.entryFile, this._platforms); const {entryFile, dev, minify, hot} = options; return this._bundler.getShallowDependencies( {entryFile, platform, dev, minify, hot, generateSourceMaps: false}, @@ -296,7 +298,7 @@ class Server { getDependencies(options: DependencyOptions): Promise> { return Promise.resolve().then(() => { const platform = options.platform != null - ? options.platform : getPlatformExtension(options.entryFile); + ? options.platform : getPlatformExtension(options.entryFile, this._platforms); const {entryFile, dev, minify, hot} = options; return this._bundler.getDependencies( {entryFile, platform, dev, minify, hot, generateSourceMaps: false}, @@ -832,7 +834,7 @@ class Server { // try to get the platform from the url /* $FlowFixMe: `query` could be empty for an invalid URL */ const platform = urlObj.query.platform || - getPlatformExtension(pathname); + getPlatformExtension(pathname, this._platforms); /* $FlowFixMe: `query` could be empty for an invalid URL */ const assetPlugin = urlObj.query.assetPlugin; diff --git a/packager/src/lib/GlobalTransformCache.js b/packager/src/lib/GlobalTransformCache.js index deb27aef9..1d2fe2c98 100644 --- a/packager/src/lib/GlobalTransformCache.js +++ b/packager/src/lib/GlobalTransformCache.js @@ -18,7 +18,6 @@ const FetchError = require('node-fetch/lib/fetch-error'); const crypto = require('crypto'); const fetch = require('node-fetch'); -const invariant = require('fbjs/lib/invariant'); const jsonStableStringify = require('json-stable-stringify'); const path = require('path'); const throat = require('throat'); @@ -134,7 +133,7 @@ class KeyResultStore { } -export type TransformProfile = {+dev: boolean, +minify: boolean, +platform: string}; +export type TransformProfile = {+dev: boolean, +minify: boolean, +platform: ?string}; function profileKey({dev, minify, platform}: TransformProfile): string { return jsonStableStringify({dev, minify, platform}); diff --git a/packager/src/node-haste/DependencyGraph.js b/packager/src/node-haste/DependencyGraph.js index bade0b2cb..c0e547f10 100644 --- a/packager/src/node-haste/DependencyGraph.js +++ b/packager/src/node-haste/DependencyGraph.js @@ -291,7 +291,6 @@ class DependencyGraph extends EventEmitter { static Module; static Polyfill; static getAssetDataFromName; - static getPlatformExtension; static replacePatterns; static getInverseDependencies; @@ -301,7 +300,6 @@ Object.assign(DependencyGraph, { Module, Polyfill, getAssetDataFromName, - getPlatformExtension, replacePatterns, getInverseDependencies, }); diff --git a/packager/src/node-haste/lib/__tests__/getAssetDataFromName-test.js b/packager/src/node-haste/lib/__tests__/getAssetDataFromName-test.js index a1e6b8cc6..546facb71 100644 --- a/packager/src/node-haste/lib/__tests__/getAssetDataFromName-test.js +++ b/packager/src/node-haste/lib/__tests__/getAssetDataFromName-test.js @@ -13,9 +13,11 @@ jest.dontMock('../getPlatformExtension') var getAssetDataFromName = require('../getAssetDataFromName'); +const TEST_PLATFORMS = new Set(['ios', 'android']); + describe('getAssetDataFromName', () => { it('should get data from name', () => { - expect(getAssetDataFromName('a/b/c.png')).toEqual({ + expect(getAssetDataFromName('a/b/c.png', TEST_PLATFORMS)).toEqual({ resolution: 1, assetName: 'a/b/c.png', type: 'png', @@ -23,7 +25,7 @@ describe('getAssetDataFromName', () => { platform: null, }); - expect(getAssetDataFromName('a/b/c@1x.png')).toEqual({ + expect(getAssetDataFromName('a/b/c@1x.png', TEST_PLATFORMS)).toEqual({ resolution: 1, assetName: 'a/b/c.png', type: 'png', @@ -31,7 +33,7 @@ describe('getAssetDataFromName', () => { platform: null, }); - expect(getAssetDataFromName('a/b/c@2.5x.png')).toEqual({ + expect(getAssetDataFromName('a/b/c@2.5x.png', TEST_PLATFORMS)).toEqual({ resolution: 2.5, assetName: 'a/b/c.png', type: 'png', @@ -39,7 +41,7 @@ describe('getAssetDataFromName', () => { platform: null, }); - expect(getAssetDataFromName('a/b/c.ios.png')).toEqual({ + expect(getAssetDataFromName('a/b/c.ios.png', TEST_PLATFORMS)).toEqual({ resolution: 1, assetName: 'a/b/c.png', type: 'png', @@ -47,7 +49,7 @@ describe('getAssetDataFromName', () => { platform: 'ios', }); - expect(getAssetDataFromName('a/b/c@1x.ios.png')).toEqual({ + expect(getAssetDataFromName('a/b/c@1x.ios.png', TEST_PLATFORMS)).toEqual({ resolution: 1, assetName: 'a/b/c.png', type: 'png', @@ -55,7 +57,7 @@ describe('getAssetDataFromName', () => { platform: 'ios', }); - expect(getAssetDataFromName('a/b/c@2.5x.ios.png')).toEqual({ + expect(getAssetDataFromName('a/b/c@2.5x.ios.png', TEST_PLATFORMS)).toEqual({ resolution: 2.5, assetName: 'a/b/c.png', type: 'png', @@ -63,7 +65,7 @@ describe('getAssetDataFromName', () => { platform: 'ios', }); - expect(getAssetDataFromName('a/b /c.png')).toEqual({ + expect(getAssetDataFromName('a/b /c.png', TEST_PLATFORMS)).toEqual({ resolution: 1, assetName: 'a/b /c.png', type: 'png', @@ -74,7 +76,7 @@ describe('getAssetDataFromName', () => { describe('resolution extraction', () => { it('should extract resolution simple case', () => { - var data = getAssetDataFromName('test@2x.png'); + var data = getAssetDataFromName('test@2x.png', TEST_PLATFORMS); expect(data).toEqual({ assetName: 'test.png', resolution: 2, @@ -85,7 +87,7 @@ describe('getAssetDataFromName', () => { }); it('should default resolution to 1', () => { - var data = getAssetDataFromName('test.png'); + var data = getAssetDataFromName('test.png', TEST_PLATFORMS); expect(data).toEqual({ assetName: 'test.png', resolution: 1, @@ -96,7 +98,7 @@ describe('getAssetDataFromName', () => { }); it('should support float', () => { - var data = getAssetDataFromName('test@1.1x.png'); + var data = getAssetDataFromName('test@1.1x.png', TEST_PLATFORMS); expect(data).toEqual({ assetName: 'test.png', resolution: 1.1, @@ -105,7 +107,7 @@ describe('getAssetDataFromName', () => { platform: null, }); - data = getAssetDataFromName('test@.1x.png'); + data = getAssetDataFromName('test@.1x.png', TEST_PLATFORMS); expect(data).toEqual({ assetName: 'test.png', resolution: 0.1, @@ -114,7 +116,7 @@ describe('getAssetDataFromName', () => { platform: null, }); - data = getAssetDataFromName('test@0.2x.png'); + data = getAssetDataFromName('test@0.2x.png', TEST_PLATFORMS); expect(data).toEqual({ assetName: 'test.png', resolution: 0.2, diff --git a/packager/src/node-haste/lib/__tests__/getPlatformExtension-test.js b/packager/src/node-haste/lib/__tests__/getPlatformExtension-test.js index ac6012368..5385e9b31 100644 --- a/packager/src/node-haste/lib/__tests__/getPlatformExtension-test.js +++ b/packager/src/node-haste/lib/__tests__/getPlatformExtension-test.js @@ -12,21 +12,20 @@ jest.dontMock('../getPlatformExtension'); var getPlatformExtension = require('../getPlatformExtension'); +const TEST_PLATFORMS = new Set(['ios', 'android']); + describe('getPlatformExtension', function() { it('should get platform ext', function() { - expect(getPlatformExtension('a.ios.js')).toBe('ios'); - expect(getPlatformExtension('a.android.js')).toBe('android'); - expect(getPlatformExtension('/b/c/a.ios.js')).toBe('ios'); - expect(getPlatformExtension('/b/c.android/a.ios.js')).toBe('ios'); - expect(getPlatformExtension('/b/c/a@1.5x.ios.png')).toBe('ios'); - expect(getPlatformExtension('/b/c/a@1.5x.lol.png')).toBe(null); - expect(getPlatformExtension('/b/c/a.lol.png')).toBe(null); - }); - - it('should optionally accept supported platforms', function() { + const get = name => getPlatformExtension(name, TEST_PLATFORMS); + expect(get('a.ios.js')).toBe('ios'); + expect(get('a.android.js')).toBe('android'); + expect(get('/b/c/a.ios.js')).toBe('ios'); + expect(get('/b/c.android/a.ios.js')).toBe('ios'); + expect(get('/b/c/a@1.5x.ios.png')).toBe('ios'); + expect(get('/b/c/a@1.5x.lol.png')).toBe(null); + expect(get('/b/c/a.lol.png')).toBe(null); expect(getPlatformExtension('a.ios.js', new Set(['ios']))).toBe('ios'); expect(getPlatformExtension('a.android.js', new Set(['android']))).toBe('android'); - expect(getPlatformExtension('/b/c/a.ios.js', new Set(['ios', 'android']))).toBe('ios'); expect(getPlatformExtension('a.ios.js', new Set(['ubuntu']))).toBe(null); expect(getPlatformExtension('a.ubuntu.js', new Set(['ubuntu']))).toBe('ubuntu'); }); diff --git a/packager/src/node-haste/lib/getPlatformExtension.js b/packager/src/node-haste/lib/getPlatformExtension.js index bc3bf7f81..85564a7f6 100644 --- a/packager/src/node-haste/lib/getPlatformExtension.js +++ b/packager/src/node-haste/lib/getPlatformExtension.js @@ -7,21 +7,15 @@ * of patent rights can be found in the PATENTS file in the same directory. * * @flow + * @format */ 'use strict'; -const SUPPORTED_PLATFORM_EXTS = new Set([ - 'android', - 'ios', - 'web', -]); - -// Extract platform extension: index.ios.js -> ios -function getPlatformExtension( - file: string, - platforms: Set = SUPPORTED_PLATFORM_EXTS, -): ?string { +/** + * Extract platform extension: `index.ios.js` -> `ios`. + */ +function getPlatformExtension(file: string, platforms: Set): ?string { const last = file.lastIndexOf('.'); const secondToLast = file.lastIndexOf('.', last - 1); if (secondToLast === -1) {