Enable code splitting for device code (#139)

Signed-off-by: Liam McLoughlin <lmcloughlin@fitbit.com>

package.json

@@ -27,29 +27,29 @@
     "@types/glob": "^7.1.1",
     "@types/gulp-zip": "^4.0.0",
     "@types/indent-string": "^4.0.1",
-    "@types/inquirer": "^6.0.3",
-    "@types/jest": "^24.0.15",
-    "@types/lodash": "^4.14.136",
+    "@types/inquirer": "^6.5.0",
+    "@types/jest": "^24.0.17",
+    "@types/lodash": "^4.14.137",
     "@types/merge-stream": "^1.1.1",
     "@types/multistream": "^2.1.1",
     "@types/plugin-error": "^0.1.0",
     "@types/pumpify": "^1.4.1",
     "@types/semver": "^6.0.1",
     "@types/update-notifier": "^2.5.0",
     "@types/uuid": "^3.4.5",
-    "@types/validator": "^10.11.1",
+    "@types/validator": "^10.11.2",
     "@types/vinyl": "^2.0.2",
     "@types/vinyl-buffer": "^1.0.0",
     "@types/vinyl-fs": "^2.4.10",
-    "@types/yargs": "^13.0.0",
+    "@types/yargs": "^13.0.2",
     "concat-stream": "^2.0.0",
     "coveralls": "^3.0.5",
     "depcheck": "^0.8.3",
     "event-stream": "^4.0.1",
     "husky": "^3.0.1",
     "jest": "^24.8.0",
     "jest-date-mock": "^1.0.7",
-    "lint-staged": "^9.2.0",
+    "lint-staged": "^9.2.1",
     "prettier": "^1.18.2",
     "ts-jest": "^24.0.0",
     "tslint": "^5.18.0",
@@ -70,32 +70,33 @@
     "dateformat": "^3.0.3",
     "elfy": "^0.1.0",
     "events-intercept": "^2.0.0",
-    "fp-ts": "^2.0.2",
+    "fp-ts": "^2.0.5",
     "fs-extra": "^8.1.0",
     "glob": "^7.1.4",
     "gulp-file": "^0.4.0",
     "gulp-zip": "^5.0.0",
     "humanize-list": "^1.0.1",
     "indent-string": "^4.0.0",
-    "inquirer": "^6.5.0",
+    "inquirer": "^6.5.1",
     "io-ts": "2.0.1",
     "lazystream": "^1.0.0",
     "lodash": "^4.17.15",
+    "magic-string": "^0.25.3",
     "merge-stream": "^2.0.0",
     "multistream": "^4.0.0",
     "plugin-error": "^1.0.1",
     "pofile": "^1.1.0",
     "pumpify": "^2.0.0",
-    "rollup": "^1.17.0",
+    "rollup": "^1.19.4",
     "rollup-plugin-babel": "^4.3.3",
-    "rollup-plugin-commonjs": "^10.0.1",
+    "rollup-plugin-commonjs": "^10.0.2",
     "rollup-plugin-node-resolve": "^5.2.0",
     "rollup-pluginutils": "^2.8.1",
-    "semver": "^6.2.0",
+    "semver": "^6.3.0",
     "simple-random": "^1.0.3",
     "source-map": "^0.7.3",
     "source-map-compactor": "^1.0.1",
-    "terser": "^4.1.2",
+    "terser": "^4.1.4",
     "tslib": "^1.10.0",
     "typescript": "^3.5.3",
     "update-notifier": "^3.0.1",

sdk-tsconfig.json

@@ -1,6 +1,6 @@
 {
   "compilerOptions": {
-    "module": "es2015",
+    "module": "esnext",
     "noEmitHelpers": false,
     "importHelpers": true,
     "noResolve": false,

src/__snapshots__/compile.test.ts.snap

@@ -82,6 +82,22 @@ function t(i){return this instanceof t?(this.v=i,this):new t(i)}t()
 "
 `;
 
+exports[`does not allow JSON imports 1`] = `"Unexpected token (Note that you need rollup-plugin-json to import JSON files)"`;
+
+exports[`emits ES5 code for device 1`] = `
+"\\"use strict\\"
+var o=function(){return\\"foo\\"}
+console.log(o())
+"
+`;
+
+exports[`emits ES6 code 1`] = `
+"\\"use strict\\"
+const o=()=>\\"foo\\"
+console.log(o())
+"
+`;
+
 exports[`emits diagnostics given JS code with bad syntax 1`] = `
 Array [
   Object {
@@ -126,6 +142,19 @@ Array [
 ]
 `;
 
+exports[`emits multiple chunks when dynamic import are used 1`] = `
+"\\"use strict\\"
+new Promise(function(n){n(require(\\"./chunkB-b146ad81.js\\"))}).then(function(n){var e=n.chunkB
+e()})
+"
+`;
+
+exports[`emits multiple chunks when dynamic import are used 2`] = `
+"(function(){\\"use strict\\"
+function o(){console.log(\\"Hello code splitting!\\")}exports.chunkB=o})()
+"
+`;
+
 exports[`emits sourcemaps with source paths relative to the project root 1`] = `
 Array [
   "src/__test__/compile/sourcemap/foo.js",
@@ -168,19 +197,3 @@ foo()
 console.log(\\"Hello!\\")
 "
 `;
-
-exports[`when targeting SDK 4.0 does not allow JSON imports 1`] = `"Unexpected token (Note that you need rollup-plugin-json to import JSON files)"`;
-
-exports[`when targeting SDK 4.0 emits ES5 code for device 1`] = `
-"\\"use strict\\"
-var o=function(){return\\"foo\\"}
-console.log(o())
-"
-`;
-
-exports[`when targeting SDK 4.0 emits ES6 code 1`] = `
-"\\"use strict\\"
-const o=()=>\\"foo\\"
-console.log(o())
-"
-`;

src/__test__/compile/chunkA.js

@@ -0,0 +1,4 @@
+import('./chunkB')
+    .then(({ chunkB }) => {
+        chunkB();
+    })

src/__test__/compile/chunkB.js

@@ -0,0 +1,3 @@
+export function chunkB() {
+    console.log('Hello code splitting!');
+}

src/compile.test.ts

@@ -7,6 +7,7 @@ import Vinyl from 'vinyl';
 import compile from './compile';
 import sdkVersion from './sdkVersion';
 import getFileFromStream from './testUtils/getFileFromStream';
+import getFilesFromStream from './testUtils/getFilesFromStream';
 import getVinylContents from './testUtils/getVinylContents';
 import { ComponentType } from './componentTargets';
 
@@ -22,7 +23,7 @@ const mockSDKVersion = sdkVersion as jest.Mock;
 
 beforeEach(() => {
   mockDiagnosticHandler = jest.fn();
-  mockSDKVersion.mockReturnValue({ major: 2, minor: 0 });
+  mockSDKVersion.mockReturnValue({ major: 4, minor: 0 });
 
   // We don't want to load the actual tsconfig.json for this project
   // during unit tests. Using a real tsconfig.json located within
@@ -106,39 +107,33 @@ it.each([ComponentType.DEVICE, ComponentType.COMPANION])(
     expect(compileFile('importImage.js', { component })).rejects.toThrowError(),
 );
 
-describe('when targeting SDK 4.0', () => {
-  beforeEach(() => mockSDKVersion.mockReturnValue({ major: 4, minor: 0 }));
+it('does not allow JSON imports', () =>
+  expect(
+    compileFile('importJSON.js').then(getVinylContents),
+  ).rejects.toThrowErrorMatchingSnapshot());
 
-  it('does not allow JSON imports', () =>
-    expect(
-      compileFile('importJSON.js').then(getVinylContents),
-    ).rejects.toThrowErrorMatchingSnapshot());
+it('does not allow unintentionally non-relative imports', () =>
+  expect(
+    compileFile('incorrectlyNonRelativeImport.js').then(getVinylContents),
+  ).rejects.toThrowError());
 
-  it('does not allow unintentionally non-relative imports', () =>
-    expect(
-      compileFile('incorrectlyNonRelativeImport.js').then(getVinylContents),
-    ).rejects.toThrowError());
+it('emits ES6 code', () =>
+  expect(
+    compileFile('ES6.js').then(getVinylContents),
+  ).resolves.toMatchSnapshot());
 
-  it('emits ES6 code', () =>
-    expect(
-      compileFile('ES6.js').then(getVinylContents),
-    ).resolves.toMatchSnapshot());
+it('emits ES5 code for device', () =>
+  expect(
+    compileFile('ES6.js', { component: ComponentType.DEVICE }).then(
+      getVinylContents,
+    ),
+  ).resolves.toMatchSnapshot());
 
-  it('emits ES5 code for device', () =>
-    expect(
-      compileFile('ES6.js', { component: ComponentType.DEVICE }).then(
-        getVinylContents,
-      ),
-    ).resolves.toMatchSnapshot());
-
-  it.each([ComponentType.DEVICE, ComponentType.COMPANION])(
-    'does not allow importing image files when building %s',
-    (component: ComponentType) =>
-      expect(
-        compileFile('importImage.js', { component }),
-      ).rejects.toBeDefined(),
-  );
-});
+it.each([ComponentType.DEVICE, ComponentType.COMPANION])(
+  'does not allow importing image files when building %s',
+  (component: ComponentType) =>
+    expect(compileFile('importImage.js', { component })).rejects.toBeDefined(),
+);
 
 it.each([
   ['an unrecognized binary file is imported', 'importBinary.js'],
@@ -225,3 +220,20 @@ it('applies specified polyfills', () => {
     }).then(getVinylContents),
   ).resolves.toMatchSnapshot();
 });
+
+it('emits multiple chunks when dynamic import are used', async () => {
+  const buildStream = compile({
+    component: ComponentType.DEVICE,
+    outputDir: 'app',
+    entryPoint: testResourcePath('chunkA.js'),
+    onDiagnostic: mockDiagnosticHandler,
+    defaultLanguage: 'en-US',
+  });
+
+  const files = await getFilesFromStream(buildStream);
+
+  const entryJS = await getVinylContents(files[0]);
+  const chunkJS = await getVinylContents(files[1]);
+  expect(entryJS).toMatchSnapshot();
+  expect(chunkJS).toMatchSnapshot();
+});

src/compile.ts

@@ -13,13 +13,15 @@ import {
   logDiagnosticToConsole,
 } from './diagnostics';
 import rollupToVinyl from './rollupToVinyl';
+import sdkVersion from './sdkVersion';
 
 import forbidAbsoluteImport from './plugins/forbidAbsoluteImport';
 import i18nPolyfill from './plugins/i18nPolyfill';
 import platformExternals from './plugins/platformExternals';
 import polyfill, { PolyfillMap } from './plugins/polyfill';
 import polyfillDevice from './plugins/polyfillDevice';
 import resourceImports from './plugins/resourceImports';
+import workaroundRequireScope from './plugins/workaroundRequireScope';
 import terser from './plugins/terser';
 import typescript from './plugins/typescript';
 import rollupWarningHandler from './rollupWarningHandler';
@@ -33,7 +35,7 @@ const tsconfigOverrides = {
   noEmit: false,
   inlineSourceMap: false,
   moduleResolution: ts.ModuleResolutionKind.NodeJs,
-  module: ts.ModuleKind.ES2015,
+  module: ts.ModuleKind.ESNext,
   suppressOutputPathCheck: true,
 };
 
@@ -101,6 +103,8 @@ export default function compile({
             inputSourceMap: false as any,
           }),
         ),
+        // Must come before terser in order not to wrap strict directive
+        ...pluginIf(sdkVersion().major >= 4, workaroundRequireScope),
         terser({
           ecma,
           // We still support iOS 10, which ships Safari 10
@@ -126,7 +130,9 @@ export default function compile({
           ? { UNRESOLVED_IMPORT: DiagnosticCategory.Warning }
           : undefined,
       }),
-      inlineDynamicImports: true,
+      // Companion/Settings have no FS and therefore can't use code splitting
+      inlineDynamicImports:
+        sdkVersion().major < 4 || component !== ComponentType.DEVICE,
     },
     {
       dir: outputDir,

src/plugins/workaroundRequireScope.ts

@@ -0,0 +1,30 @@
+import MagicString from 'magic-string';
+import { Plugin } from 'rollup';
+
+// require is bugged in FbOS such that the loaded module has its scope
+// merged with the global scope, which is bad news (and particularly
+// so if you use a minifier as we do). In order to workaround this,
+// this plugin wraps all non-entry chunks in an IIFE to maintain
+// isolation of scopes.
+
+export default function workaroundChunkScope(): Plugin {
+  return {
+    name: 'workaroundChunkScope',
+
+    renderChunk(code, chunk) {
+      // Only non-entry chunks need this fix (since those are loaded using require)
+      if (chunk.isEntry) return null;
+
+      const magic = new MagicString(code);
+      magic.prepend('(function(){');
+      magic.append('})()');
+
+      return {
+        code: magic.toString(),
+        map: magic.generateMap({
+          hires: true,
+        }),
+      };
+    },
+  };
+}

src/testUtils/getFilesFromStream.ts

@@ -0,0 +1,20 @@
+import stream from 'stream';
+
+import Vinyl from 'vinyl';
+
+export default function getFilesFromStream(
+  stream: stream.Readable,
+  numFiles = 1,
+) {
+  const files: Vinyl[] = [];
+  return new Promise<Vinyl[]>((resolve, reject) => {
+    stream.on('data', (file: Vinyl) => {
+      if (file.contents) {
+        files.push(file);
+        if (files.length === numFiles) resolve(files);
+      }
+    });
+    stream.on('finish', () => reject(new Error('No file in stream matched')));
+    stream.on('error', reject);
+  });
+}

tslint.json

@@ -11,6 +11,7 @@
       true,
       {
         "errorSubclass": "ErrorSubclass",
+        "magicString": "MagicString",
         "pluginError": "PluginError",
         "typescript": "ts",
         "vinyl": "Vinyl",