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

@@ -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<string>,
     |},
     ): Promise<BundlingOptions> {

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 = {

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,
 |};
 

packager/src/Resolver/index.js

@@ -98,7 +98,7 @@ class Resolver {
 
   getDependencies<T: ContainsTransformerOptions>(
     entryPath: string,
-    options: {platform: string, recursive?: boolean},
+    options: {platform: ?string, recursive?: boolean},
     bundlingOptions: T,
     onProgress?: ?(finishedModules: number, totalModules: number) => mixed,
     getModuleId: mixed,

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,

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<string>;
 
   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<Array<Module>> {
     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<ResolutionResponse<Module, *>> {
     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;

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});

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,
 });

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,

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');
   });

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<string> = SUPPORTED_PLATFORM_EXTS,
-): ?string {
+/**
+ * Extract platform extension: `index.ios.js` -> `ios`.
+ */
+function getPlatformExtension(file: string, platforms: Set<string>): ?string {
   const last = file.lastIndexOf('.');
   const secondToLast = file.lastIndexOf('.', last - 1);
   if (secondToLast === -1) {