packager: simplify getAssetDataFromName?

Summary:
Not only is this function is building a custom Regex for every single file, but it's also duplicating some of the work of the inner function, that is already splitting up the platform/extension. This changeset refactors both function to have a more strict and legible logic to extract asset information. We extract the base name first, then we extract the resolution from it, instead of rematching platform and extension.

I stumbled on this while looking into refactoring the asset resolution logic of `ResolutionRequest#_loadAsAssetFile()`. The next step would be to reuse the exact same function for resolving assets instead of using a custom regex there as well.

Reviewed By: davidaurelio

Differential Revision: D5060589

fbshipit-source-id: b48d9a5d8e963be76cad5384c6bfb3e214a73587

packager/src/AssetServer/__tests__/AssetServer-test.js

@@ -20,12 +20,6 @@ const fs = require('fs');
 const {objectContaining} = jasmine;
 
 describe('AssetServer', () => {
-  beforeEach(() => {
-    const NodeHaste = require('../../node-haste/DependencyGraph');
-    NodeHaste.getAssetDataFromName =
-      require.requireActual('../../node-haste/lib/getAssetDataFromName');
-  });
-
   describe('assetServer.get', () => {
     it('should work for the simple case', () => {
       const server = new AssetServer({

packager/src/AssetServer/index.js

@@ -11,13 +11,14 @@
 
 'use strict';
 
+const AssetPaths = require('../node-haste/lib/AssetPaths');
+
 const crypto = require('crypto');
 const denodeify = require('denodeify');
 const fs = require('fs');
-const getAssetDataFromName = require('../node-haste/lib/getAssetDataFromName');
 const path = require('path');
 
-import type {AssetData} from '../node-haste/lib/getAssetDataFromName';
+import type {AssetData} from '../node-haste/lib/AssetPaths';
 
 const createTimeoutPromise = timeout => new Promise((resolve, reject) => {
   setTimeout(reject, timeout, 'fs operation timeout');
@@ -55,7 +56,7 @@ class AssetServer {
   }
 
   get(assetPath: string, platform: ?string = null): Promise<Buffer> {
-    const assetData = getAssetDataFromName(
+    const assetData = AssetPaths.parse(
       assetPath,
       new Set(platform != null ? [platform] : []),
     );
@@ -77,7 +78,7 @@ class AssetServer {
     scales: Array<number>,
     type: string,
   |}> {
-    const nameData = getAssetDataFromName(
+    const nameData = AssetPaths.parse(
       assetPath,
       new Set(platform != null ? [platform] : []),
     );
@@ -141,7 +142,7 @@ class AssetServer {
       .then(res => {
         const dir = res[0];
         const files = res[1];
-        const assetData = getAssetDataFromName(
+        const assetData = AssetPaths.parse(
           filename,
           new Set(platform != null ? [platform] : []),
         );
@@ -209,6 +210,9 @@ class AssetServer {
     const assets = files.map(this._getAssetDataFromName.bind(this, platforms));
     const map = new Map();
     assets.forEach(function(asset, i) {
+      if (asset == null) {
+        return;
+      }
       const file = files[i];
       const assetKey = getAssetKey(asset.assetName, asset.platform);
       let record = map.get(assetKey);
@@ -235,8 +239,8 @@ class AssetServer {
     return map;
   }
 
-  _getAssetDataFromName(platforms: Set<string>, file: string): AssetData {
-    return getAssetDataFromName(file, platforms);
+  _getAssetDataFromName(platforms: Set<string>, file: string): ?AssetData {
+    return AssetPaths.tryParse(file, platforms);
   }
 }
 

packager/src/Server/index.js

@@ -16,8 +16,8 @@ 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 parsePlatformFilePath = require('../node-haste/lib/parsePlatformFilePath');
 const path = require('path');
 const symbolicate = require('./symbolicate');
 const terminal = require('../lib/terminal');
@@ -283,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, this._platforms);
+        ? options.platform : parsePlatformFilePath(options.entryFile, this._platforms).platform;
       const {entryFile, dev, minify, hot} = options;
       return this._bundler.getShallowDependencies(
         {entryFile, platform, dev, minify, hot, generateSourceMaps: false},
@@ -298,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, this._platforms);
+        ? options.platform : parsePlatformFilePath(options.entryFile, this._platforms).platform;
       const {entryFile, dev, minify, hot} = options;
       return this._bundler.getDependencies(
         {entryFile, platform, dev, minify, hot, generateSourceMaps: false},
@@ -834,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, this._platforms);
+      parsePlatformFilePath(pathname, this._platforms).platform;
 
     /* $FlowFixMe: `query` could be empty for an invalid URL */
     const assetPlugin = urlObj.query.assetPlugin;

packager/src/node-haste/AssetModule.js

@@ -12,10 +12,9 @@
 
 'use strict';
 
+const AssetPaths = require('./lib/AssetPaths');
 const Module = require('./Module');
 
-const getAssetDataFromName = require('./lib/getAssetDataFromName');
-
 import type {CachedReadResult, ConstructorArgs, ReadResult} from './Module';
 
 class AssetModule extends Module {
@@ -29,7 +28,7 @@ class AssetModule extends Module {
     platforms: Set<string>,
   ) {
     super(args);
-    const {resolution, name, type} = getAssetDataFromName(this.path, platforms);
+    const {resolution, name, type} = AssetPaths.parse(this.path, platforms);
     this.resolution = resolution;
     this._name = name;
     this._type = type;

packager/src/node-haste/DependencyGraph.js

@@ -20,9 +20,9 @@ const ResolutionRequest = require('./DependencyGraph/ResolutionRequest');
 const ResolutionResponse = require('./DependencyGraph/ResolutionResponse');
 
 const fs = require('fs');
-const getPlatformExtension = require('./lib/getPlatformExtension');
 const invariant = require('fbjs/lib/invariant');
 const isAbsolutePath = require('absolute-path');
+const parsePlatformFilePath = require('./lib/parsePlatformFilePath');
 const path = require('path');
 const util = require('util');
 
@@ -253,7 +253,7 @@ class DependencyGraph extends EventEmitter {
 
   _getRequestPlatform(entryPath: string, platform: ?string): ?string {
     if (platform == null) {
-      platform = getPlatformExtension(entryPath, this._opts.platforms);
+      platform = parsePlatformFilePath(entryPath, this._opts.platforms).platform;
     } else if (!this._opts.platforms.has(platform)) {
       throw new Error('Unrecognized platform: ' + platform);
     }

packager/src/node-haste/DependencyGraph/HasteMap.js

@@ -14,7 +14,7 @@
 
 const EventEmitter = require('events');
 
-const getPlatformExtension = require('../lib/getPlatformExtension');
+const parsePlatformFilePath = require('../lib/parsePlatformFilePath');
 const path = require('path');
 const throat = require('throat');
 
@@ -194,7 +194,9 @@ class HasteMap<TModule: Moduleish, TPackage: Packageish> extends EventEmitter {
         this._map[name] = Object.create(null);
       }
       const moduleMap = this._map[name];
-      const modulePlatform = getPlatformExtension(mod.path, this._platforms) || GENERIC_PLATFORM;
+      const modulePlatform =
+        parsePlatformFilePath(mod.path, this._platforms).platform ||
+        GENERIC_PLATFORM;
       existingModule = moduleMap[modulePlatform];
       moduleMap[modulePlatform] = mod;
     }

packager/src/node-haste/DependencyGraph/ResolutionRequest.js

@@ -12,6 +12,7 @@
 
 'use strict';
 
+const AssetPaths = require('../lib/AssetPaths');
 const AsyncTaskGroup = require('../lib/AsyncTaskGroup');
 const MapWithDefaults = require('../lib/MapWithDefaults');
 
@@ -21,7 +22,6 @@ const path = require('path');
 const realPath = require('path');
 const invariant = require('fbjs/lib/invariant');
 const isAbsolutePath = require('absolute-path');
-const getAssetDataFromName = require('../lib/getAssetDataFromName');
 
 import type {HasteFS} from '../types';
 import type DependencyGraphHelpers from './DependencyGraphHelpers';
@@ -619,7 +619,7 @@ class ResolutionRequest<TModule: Moduleish, TPackage: Packageish> {
     fromModule: TModule,
     toModule: string,
   ): TModule {
-    const {name, type} = getAssetDataFromName(potentialModulePath, EMPTY_SET);
+    const {name, type} = AssetPaths.parse(potentialModulePath, EMPTY_SET);
 
     let pattern = '^' + name + '(@[\\d\\.]+x)?';
     if (this._options.platform != null) {

packager/src/node-haste/__tests__/AssetModule-test.js

@@ -13,7 +13,7 @@ jest.autoMockOff();
 const AssetModule = require('../AssetModule');
 
 describe('AssetModule:', () => {
-  const defaults = {file: '/arbitrary'};
+  const defaults = {file: '/arbitrary.png'};
 
   it('has no dependencies by default', () => {
     return new AssetModule(defaults).getDependencies()

packager/src/node-haste/lib/AssetPaths.js

@@ -0,0 +1,80 @@
+ /**
+ * Copyright (c) 2015-present, Facebook, Inc.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD-style license found in the
+ * 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.
+ *
+ * @flow
+ * @format
+ */
+
+'use strict';
+
+const parsePlatformFilePath = require('./parsePlatformFilePath');
+const path = require('path');
+
+export type AssetData = {|
+  // TODO: rename to "assetPath", what it actually is.
+  assetName: string,
+  name: string,
+  platform: ?string,
+  resolution: number,
+  type: string,
+|};
+
+const ASSET_BASE_NAME_RE = /(.+?)(@([\d.]+)x)?$/;
+
+function parseBaseName(
+  baseName: string,
+): {rootName: string, resolution: number} {
+  const match = baseName.match(ASSET_BASE_NAME_RE);
+  if (!match) {
+    throw new Error(`invalid asset name: \`${baseName}'`);
+  }
+  const rootName = match[1];
+  if (match[3] != null) {
+    const resolution = parseFloat(match[3]);
+    if (!Number.isNaN(resolution)) {
+      return {rootName, resolution};
+    }
+  }
+  return {rootName, resolution: 1};
+}
+
+/**
+ * Return `null` if the `filePath` doesn't have a valid extension, required
+ * to describe the type of an asset.
+ */
+function tryParse(
+  filePath: string,
+  platforms: Set<string>,
+): ?AssetData {
+  const result = parsePlatformFilePath(filePath, platforms);
+  const {dirPath, baseName, platform, extension} = result;
+  if (extension == null) {
+    return null;
+  }
+  const {rootName, resolution} = parseBaseName(baseName);
+  return {
+    assetName: path.join(dirPath, `${rootName}.${extension}`),
+    name: rootName,
+    platform,
+    resolution: resolution,
+    type: extension,
+  };
+}
+
+function parse(
+  filePath: string,
+  platforms: Set<string>,
+): AssetData {
+  const result = tryParse(filePath, platforms);
+  if (result == null) {
+    throw new Error('invalid asset file path: \`${filePath}');
+  }
+  return result;
+}
+
+module.exports = {parse, tryParse};

packager/src/node-haste/lib/__tests__/AssetPaths-test.js

@@ -8,16 +8,16 @@
  */
 'use strict';
 
-jest.dontMock('../getPlatformExtension')
-    .dontMock('../getAssetDataFromName');
+jest.dontMock('../parsePlatformFilePath')
+    .dontMock('../AssetPaths');
 
-var getAssetDataFromName = require('../getAssetDataFromName');
+var AssetPaths = require('../AssetPaths');
 
 const TEST_PLATFORMS = new Set(['ios', 'android']);
 
-describe('getAssetDataFromName', () => {
+describe('AssetPaths', () => {
   it('should get data from name', () => {
-    expect(getAssetDataFromName('a/b/c.png', TEST_PLATFORMS)).toEqual({
+    expect(AssetPaths.parse('a/b/c.png', TEST_PLATFORMS)).toEqual({
       resolution: 1,
       assetName: 'a/b/c.png',
       type: 'png',
@@ -25,7 +25,7 @@ describe('getAssetDataFromName', () => {
       platform: null,
     });
 
-    expect(getAssetDataFromName('a/b/c@1x.png', TEST_PLATFORMS)).toEqual({
+    expect(AssetPaths.parse('a/b/c@1x.png', TEST_PLATFORMS)).toEqual({
       resolution: 1,
       assetName: 'a/b/c.png',
       type: 'png',
@@ -33,7 +33,7 @@ describe('getAssetDataFromName', () => {
       platform: null,
     });
 
-    expect(getAssetDataFromName('a/b/c@2.5x.png', TEST_PLATFORMS)).toEqual({
+    expect(AssetPaths.parse('a/b/c@2.5x.png', TEST_PLATFORMS)).toEqual({
       resolution: 2.5,
       assetName: 'a/b/c.png',
       type: 'png',
@@ -41,7 +41,7 @@ describe('getAssetDataFromName', () => {
       platform: null,
     });
 
-    expect(getAssetDataFromName('a/b/c.ios.png', TEST_PLATFORMS)).toEqual({
+    expect(AssetPaths.parse('a/b/c.ios.png', TEST_PLATFORMS)).toEqual({
       resolution: 1,
       assetName: 'a/b/c.png',
       type: 'png',
@@ -49,7 +49,7 @@ describe('getAssetDataFromName', () => {
       platform: 'ios',
     });
 
-    expect(getAssetDataFromName('a/b/c@1x.ios.png', TEST_PLATFORMS)).toEqual({
+    expect(AssetPaths.parse('a/b/c@1x.ios.png', TEST_PLATFORMS)).toEqual({
       resolution: 1,
       assetName: 'a/b/c.png',
       type: 'png',
@@ -57,7 +57,7 @@ describe('getAssetDataFromName', () => {
       platform: 'ios',
     });
 
-    expect(getAssetDataFromName('a/b/c@2.5x.ios.png', TEST_PLATFORMS)).toEqual({
+    expect(AssetPaths.parse('a/b/c@2.5x.ios.png', TEST_PLATFORMS)).toEqual({
       resolution: 2.5,
       assetName: 'a/b/c.png',
       type: 'png',
@@ -65,7 +65,7 @@ describe('getAssetDataFromName', () => {
       platform: 'ios',
     });
 
-    expect(getAssetDataFromName('a/b /c.png', TEST_PLATFORMS)).toEqual({
+    expect(AssetPaths.parse('a/b /c.png', TEST_PLATFORMS)).toEqual({
       resolution: 1,
       assetName: 'a/b /c.png',
       type: 'png',
@@ -76,7 +76,7 @@ describe('getAssetDataFromName', () => {
 
   describe('resolution extraction', () => {
     it('should extract resolution simple case', () =>  {
-      var data = getAssetDataFromName('test@2x.png', TEST_PLATFORMS);
+      var data = AssetPaths.parse('test@2x.png', TEST_PLATFORMS);
       expect(data).toEqual({
         assetName: 'test.png',
         resolution: 2,
@@ -87,7 +87,7 @@ describe('getAssetDataFromName', () => {
     });
 
     it('should default resolution to 1', () =>  {
-      var data = getAssetDataFromName('test.png', TEST_PLATFORMS);
+      var data = AssetPaths.parse('test.png', TEST_PLATFORMS);
       expect(data).toEqual({
         assetName: 'test.png',
         resolution: 1,
@@ -98,7 +98,7 @@ describe('getAssetDataFromName', () => {
     });
 
     it('should support float', () =>  {
-      var data = getAssetDataFromName('test@1.1x.png', TEST_PLATFORMS);
+      var data = AssetPaths.parse('test@1.1x.png', TEST_PLATFORMS);
       expect(data).toEqual({
         assetName: 'test.png',
         resolution: 1.1,
@@ -107,7 +107,7 @@ describe('getAssetDataFromName', () => {
         platform: null,
       });
 
-      data = getAssetDataFromName('test@.1x.png', TEST_PLATFORMS);
+      data = AssetPaths.parse('test@.1x.png', TEST_PLATFORMS);
       expect(data).toEqual({
         assetName: 'test.png',
         resolution: 0.1,
@@ -116,7 +116,7 @@ describe('getAssetDataFromName', () => {
         platform: null,
       });
 
-      data = getAssetDataFromName('test@0.2x.png', TEST_PLATFORMS);
+      data = AssetPaths.parse('test@0.2x.png', TEST_PLATFORMS);
       expect(data).toEqual({
         assetName: 'test.png',
         resolution: 0.2,

packager/src/node-haste/lib/__tests__/parsePlatformFilePath-test.js

@@ -8,15 +8,16 @@
  */
 'use strict';
 
-jest.dontMock('../getPlatformExtension');
+jest.dontMock('../parsePlatformFilePath');
 
-var getPlatformExtension = require('../getPlatformExtension');
+var parsePlatformFilePath = require('../parsePlatformFilePath');
 
 const TEST_PLATFORMS = new Set(['ios', 'android']);
 
-describe('getPlatformExtension', function() {
+describe('parsePlatformFilePath', function() {
   it('should get platform ext', function() {
-    const get = name => getPlatformExtension(name, TEST_PLATFORMS);
+    const get = name => parsePlatformFilePath(name, TEST_PLATFORMS).platform;
+    expect(get('a.js')).toBe(null);
     expect(get('a.ios.js')).toBe('ios');
     expect(get('a.android.js')).toBe('android');
     expect(get('/b/c/a.ios.js')).toBe('ios');
@@ -24,9 +25,9 @@ describe('getPlatformExtension', function() {
     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('a.ios.js', new Set(['ubuntu']))).toBe(null);
-    expect(getPlatformExtension('a.ubuntu.js', new Set(['ubuntu']))).toBe('ubuntu');
+    expect(parsePlatformFilePath('a.ios.js', new Set(['ios'])).platform).toBe('ios');
+    expect(parsePlatformFilePath('a.android.js', new Set(['android'])).platform).toBe('android');
+    expect(parsePlatformFilePath('a.ios.js', new Set(['ubuntu'])).platform).toBe(null);
+    expect(parsePlatformFilePath('a.ubuntu.js', new Set(['ubuntu'])).platform).toBe('ubuntu');
   });
 });

packager/src/node-haste/lib/getAssetDataFromName.js

@@ -1,67 +0,0 @@
- /**
- * Copyright (c) 2015-present, Facebook, Inc.
- * All rights reserved.
- *
- * This source code is licensed under the BSD-style license found in the
- * 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.
- *
- * @flow
- */
-
-'use strict';
-
-const getPlatformExtension = require('./getPlatformExtension');
-const path = require('path');
-
-export type AssetData = {|
-  assetName: string,
-  name: string,
-  platform: ?string,
-  resolution: number,
-  type: string,
-|};
-
-function getAssetDataFromName(filename: string, platforms: Set<string>): AssetData {
-  const ext = path.extname(filename);
-  const platformExt = getPlatformExtension(filename, platforms);
-
-  let pattern = '@([\\d\\.]+)x';
-  if (platformExt != null) {
-    pattern += '(\\.' + platformExt + ')?';
-  }
-  pattern += '\\' + ext + '$';
-  const re = new RegExp(pattern);
-
-  const match = filename.match(re);
-  let resolution;
-
-  if (!(match && match[1])) {
-    resolution = 1;
-  } else {
-    resolution = parseFloat(match[1]);
-    if (isNaN(resolution)) {
-      resolution = 1;
-    }
-  }
-
-  let assetName;
-  if (match) {
-    assetName = filename.replace(re, ext);
-  } else if (platformExt != null) {
-    assetName = filename.replace(new RegExp(`\\.${platformExt}\\${ext}`), ext);
-  } else {
-    assetName = filename;
-  }
-  assetName = decodeURIComponent(assetName);
-
-  return {
-    assetName,
-    name: path.basename(assetName, ext),
-    platform: platformExt,
-    resolution,
-    type: ext.slice(1),
-  };
-}
-
-module.exports = getAssetDataFromName;

packager/src/node-haste/lib/getPlatformExtension.js

@@ -1,28 +0,0 @@
-/**
- * Copyright (c) 2015-present, Facebook, Inc.
- * All rights reserved.
- *
- * This source code is licensed under the BSD-style license found in the
- * 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.
- *
- * @flow
- * @format
- */
-
-'use strict';
-
-/**
- * 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) {
-    return null;
-  }
-  const platform = file.substring(secondToLast + 1, last);
-  return platforms.has(platform) ? platform : null;
-}
-
-module.exports = getPlatformExtension;

packager/src/node-haste/lib/parsePlatformFilePath.js

@@ -0,0 +1,49 @@
+/**
+ * Copyright (c) 2015-present, Facebook, Inc.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD-style license found in the
+ * 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.
+ *
+ * @flow
+ * @format
+ */
+
+'use strict';
+
+const path = require('path');
+
+type PlatformFilePathParts = {|
+  dirPath: string,
+  baseName: string,
+  platform: ?string,
+  extension: ?string,
+|};
+
+const PATH_RE = /^(.+?)(\.([^.]+))?\.([^.]+)$/;
+
+/**
+ * Extract the components of a file path that can have a platform specifier: Ex.
+ * `index.ios.js` is specific to the `ios` platform and has the extension `js`.
+ */
+function parsePlatformFilePath(
+  filePath: string,
+  platforms: Set<string>,
+): PlatformFilePathParts {
+  const dirPath = path.dirname(filePath);
+  const fileName = path.basename(filePath);
+  const match = fileName.match(PATH_RE);
+  if (!match) {
+    return {dirPath, baseName: fileName, platform: null, extension: null};
+  }
+  const extension = match[4] || null;
+  const platform = match[3] || null;
+  if (platform == null || platforms.has(platform)) {
+    return {dirPath, baseName: match[1], platform, extension};
+  }
+  const baseName = `${match[1]}.${platform}`;
+  return {dirPath, baseName, platform: null, extension};
+}
+
+module.exports = parsePlatformFilePath;