Code-cleanup of splittable fork. (ampproject#16846)

- Removes support for browser masking in NPM.
- Simplify code based on lack of base module separate from main.
- Consolidates file patching in update-packages gulp task.
- Lots of other small stuff
- As a follow up to ampproject#16837 switch document-register-element to not use a default export from CommonJS, because this confuses our build system across babel, browserify, closure, etc.

build-system/config.js

@@ -124,6 +124,7 @@ module.exports = {
         'dist.3p/[0-9]*,dist.3p/current,dist.3p/current-min}/**/*.*',
     '!dist.3p/current/**/ampcontext-lib.js',
     '!dist.3p/current/**/iframe-transport-client-lib.js',
+    '!out/**/*.*',
     '!validator/dist/**/*.*',
     '!validator/node_modules/**/*.*',
     '!validator/nodejs/node_modules/**/*.*',

build-system/get-dep-graph.js

@@ -19,7 +19,6 @@ const browserify = require('browserify');
 const ClosureCompiler = require('google-closure-compiler').compiler;
 const devnull = require('dev-null');
 const fs = require('fs-extra');
-const mkpath = require('mkpath');
 const move = require('glob-move');
 const path = require('path');
 const Promise = require('bluebird');
@@ -51,9 +50,6 @@ let extensions = extensionBundles.filter(unsupportedExtensions).map(ext => {
 // Flatten nested arrays to support multiple versions
 extensions = [].concat.apply([], extensions);
 
-patchWebAnimations();
-patchDompurify();
-
 exports.splittable = function(config) {
 
   if (!config || !config.modules) {
@@ -152,10 +148,8 @@ exports.getBundleFlags = function(g) {
   // Write all the packages (directories with a package.json) as --js
   // inputs to the flags. Closure compiler reads the packages to resolve
   // non-relative module names.
-  let packageCount = 0;
   Object.keys(g.packages).sort().forEach(function(pkg) {
     flagsArray.push('--js', pkg);
-    packageCount++;
   });
 
   // Build up the weird flag structure that closure compiler calls
@@ -171,32 +165,11 @@ exports.getBundleFlags = function(g) {
   bundleKeys.splice(1, 0, '_base_i');
   bundleKeys.forEach(function(originalName) {
     const isMain = originalName == mainBundle;
-    const isBase = isMain;
-    let extraModules = 0;
     // TODO(erwinm): This access will break
     const bundle = g.bundles[originalName];
-    if (isBase || bundleKeys.length == 1) {
-      flagsArray.push('--js', relativePath(process.cwd(),
-          require.resolve('./base.js')));
-      extraModules++;
-      Object.keys(g.browserMask).sort().forEach(function(mask) {
-        flagsArray.push('--js', mask);
-        extraModules++;
-      });
-    }
     bundle.modules.forEach(function(js) {
       flagsArray.push('--js', js);
     });
-    if (!isBase && bundleKeys.length > 1) {
-      // TODO(erwinm): do we still need the bundleTrailModule?
-      //flagsArray.push('--js', bundleTrailModule(bundle.name));
-      //extraModules++;
-    }
-    // The packages count as inputs to the first module.
-    if (packageCount) {
-      extraModules += packageCount;
-      packageCount = 0;
-    }
     let name;
     let info = extensionsInfo[bundle.name];
     if (info) {
@@ -220,9 +193,9 @@ exports.getBundleFlags = function(g) {
       };
     }
     // And now build --module $name:$numberOfJsFiles:$bundleDeps
-    let cmd = name + ':' + (bundle.modules.length + extraModules);
+    let cmd = name + ':' + (bundle.modules.length);
     const bundleDeps = [];
-    if (!isBase) {
+    if (!isMain) {
       const configEntry = getExtensionBundleConfig(originalName);
       if (configEntry) {
         cmd += `:${configEntry.type}`;
@@ -287,7 +260,6 @@ exports.getGraph = function(entryModules, config) {
       },
     },
     packages: {},
-    browserMask: {},
   };
 
   TYPES_VALUES.forEach(type => {
@@ -314,47 +286,6 @@ exports.getGraph = function(entryModules, config) {
           require.resolve('babel-plugin-transform-es2015-modules-commonjs'),
         ],
       });
-
-  b.on('package', function(pkg) {
-    if (!pkg.browser) {
-      return;
-    }
-    Object.keys(pkg.browser).sort().forEach(function(entry) {
-      if (/^\./.test(entry)) {
-        throw new Error(
-            'Relative entries in package.json#browser not yet supported: ' +
-            entry + ' [' + pkg.__dirname + '.package.json]');
-      }
-      if (pkg.browser[entry] !== false) {
-        throw new Error(
-            'Only masking of entire modules via false supported in ' +
-            'package.json#browser:' + entry +
-            ' [' + pkg.__dirname + '.package.json]');
-      }
-      let filename =
-          'splittable-build/browser/node_modules/' + entry;
-      const maskedPkg = 'splittable-build/browser/node_modules/' +
-          entry.split('/')[0] + '/package.json';
-      if (!/\//.test(entry)) {
-        filename += '/index';
-      }
-      filename = exports.maybeAddDotJs(filename);
-      if (graph.browserMask[filename]) {
-        return;
-      }
-      graph.browserMask[filename] = true;
-      mkpath.sync(path.dirname(filename));
-      fs.writeFileSync(filename,
-          '// Generated to mask module via package.json#browser.\n' +
-          'module.exports = {};\n');
-      if (graph.browserMask[pkg]) {
-        return;
-      }
-      graph.browserMask[maskedPkg] = true;
-      fs.writeFileSync(maskedPkg,
-          '{"Generated to mask module via package.json#browser":true}\n');
-    });
-  });
   // This gets us the actual deps. We collect them in an array, so
   // we can sort them prior to building the dep tree. Otherwise the tree
   // will not be stable.
@@ -374,19 +305,8 @@ exports.getGraph = function(entryModules, config) {
           relativePath(process.cwd(), row.id)));
       topo.addNode(id, id);
       const deps = edges[id] = Object.keys(row.deps).sort().map(function(dep) {
-        //const depId = row.deps[dep];
-        const relPathtoDep = unifyPath(relativePath(process.cwd(),
+        return unifyPath(relativePath(process.cwd(),
             row.deps[dep]));
-
-        // TODO(erwimm): don't parse package.json, breaks closure right now.
-        // Non relative module path. Find the package.json.
-        //if (!/^\./.test(dep)) {
-        //var packageJson = findPackageJson(depId);
-        //if (packageJson) {
-        //graph.packages[packageJson] = true;
-        //}
-        //}
-        return relPathtoDep;
       });
       graph.deps[id] = deps;
       if (row.entry) {
@@ -405,11 +325,7 @@ exports.getGraph = function(entryModules, config) {
     graph.sorted = Array.from(topo.sort().keys()).reverse();
 
     setupBundles(graph);
-
     resolve(graph);
-
-    //buildUpCommon(graph);
-
     fs.writeFileSync('deps.txt', JSON.stringify(graph, null, 2));
   }).on('error', reject).pipe(devnull());
   return promise;
@@ -503,10 +419,7 @@ function unifyPath(id) {
 
 const externs = [
   'build-system/amp.extern.js',
-  //'third_party/closure-compiler/externs/intersection_observer.js',
   'third_party/closure-compiler/externs/performance_observer.js',
-  //'third_party/closure-compiler/externs/shadow_dom.js',
-  //'third_party/closure-compiler/externs/streams.js',
   'third_party/closure-compiler/externs/web_animations.js',
   'third_party/moment/moment.extern.js',
   'third_party/react-externs/externs.js',
@@ -557,30 +470,3 @@ function compile(flagsArray) {
     });
   });
 }
-
-// TODO(@cramforce): Unify with update-packages.js
-function patchWebAnimations() {
-  // Copies web-animations-js into a new file that has an export.
-  const patchedName = 'node_modules/web-animations-js/' +
-      'web-animations.install.js';
-  if (fs.existsSync(patchedName)) {
-    return;
-  }
-  let file = fs.readFileSync(
-      'node_modules/web-animations-js/' +
-      'web-animations.min.js').toString();
-  // Wrap the contents inside the install function.
-  file = 'exports.installWebAnimations = function(window) {\n' +
-      'var document = window.document;\n' +
-      file + '\n' +
-      '}\n';
-  fs.writeFileSync(patchedName, file);
-}
-
-// TODO(@cramforce): Unify with update-packages.js
-function patchDompurify() {
-  const name = 'node_modules/dompurify/package.json';
-  const file = fs.readFileSync(name).toString()
-      .replace('"browser": "dist/purify.js",', '');
-  fs.writeFileSync(name, file);
-}

build-system/tasks/presubmit-checks.js

@@ -75,6 +75,10 @@ const forbiddenTerms = {
         'https://medium.com/gulpjs/gulp-util-ca3b1f9f9ac5 ' +
         'for a list of alternatives.',
   },
+  'document-register-element.node': {
+    message: 'Use `document-register-element.patched` instead',
+    whitelist: ['build-system/tasks/update-packages.js'],
+  },
   'sinon\\.(spy|stub|mock)\\(': {
     message: 'Use a sandbox instead to avoid repeated `#restore` calls',
   },

build-system/tasks/update-packages.js

@@ -55,6 +55,10 @@ function patchWebAnimations() {
   }
 }
 
+/**
+ * Creates a version of document-register-element that can be installed
+ * without side effects.
+ */
 function patchRegisterElement() {
   let file;
   // Copies document-register-element into a new file that has an export.
@@ -82,14 +86,15 @@ function patchRegisterElement() {
           'to appear in document-register-element');
     }
     file = file.replace('module.exports = installCustomElements;',
-        'module.exports = exports.default = installCustomElements;');
+        'exports.installCustomElements = installCustomElements;');
     fs.writeFileSync(patchedName, file);
     if (!process.env.TRAVIS) {
       log(colors.green('Patched'), colors.cyan(patchedName));
     }
   }
 }
 
+
 /**
  * Installs custom lint rules in build-system/eslint-rules to node_modules.
  */

src/polyfills.js

@@ -24,7 +24,7 @@ import {install as installObjectAssign} from './polyfills/object-assign';
 import {install as installPromise} from './polyfills/promise';
 // Importing the document-register-element module has the side effect
 // of installing the custom elements polyfill if necessary.
-import installCustomElements from
+import {installCustomElements} from
   'document-register-element/build/document-register-element.patched';
 
 installCustomElements(self, 'auto');

src/service/extensions-impl.js

@@ -32,6 +32,8 @@ import {
 import {cssText} from '../../build/css';
 import {dev, rethrowAsync} from '../log';
 import {getMode} from '../mode';
+import {installCustomElements} from
+  'document-register-element/build/document-register-element.patched';
 import {
   install as installDOMTokenListToggle,
 } from '../polyfills/domtokenlist-toggle';
@@ -42,8 +44,6 @@ import {installPixel} from '../../builtins/amp-pixel';
 import {installStylesForDoc, installStylesLegacy} from '../style-installer';
 import {map} from '../utils/object';
 import {toWin} from '../types';
-import installCustomElements from
-  'document-register-element/build/document-register-element.patched';
 
 const TAG = 'extensions';
 const UNKNOWN_EXTENSION = '_UNKNOWN_';

test/manual/amp-single-pass-compilation.html

@@ -5,8 +5,6 @@
   <title>AMP #0</title>
   <link rel="canonical" href="amps.html" >
   <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
-  <link href='https://fonts.googleapis.com/css?family=Questrial' rel='stylesheet' type='text/css'>
-
   <style amp-custom>
     body {
       max-width: 527px;

testing/describes.js

@@ -104,6 +104,8 @@ import {
   installBuiltinElements,
   installExtensionsService,
 } from '../src/service/extensions-impl';
+import {installCustomElements} from
+  'document-register-element/build/document-register-element.patched';
 import {installDocService} from '../src/service/ampdoc-impl';
 import {installFriendlyIframeEmbed} from '../src/friendly-iframe-embed';
 import {
@@ -112,8 +114,6 @@ import {
 import {setStyles} from '../src/style';
 import {stubService} from './test-helper';
 import fetchMock from 'fetch-mock';
-import installCustomElements from
-  'document-register-element/build/document-register-element.patched';
 
 /** Should have something in the name, otherwise nothing is shown. */
 const SUB = ' ';

testing/iframe.js

@@ -25,12 +25,12 @@ import {
   installAmpdocServices,
   installRuntimeServices,
 } from '../src/runtime';
+import {installCustomElements} from
+  'document-register-element/build/document-register-element.patched';
 import {installDocService} from '../src/service/ampdoc-impl';
 import {installExtensionsService} from '../src/service/extensions-impl';
 import {installStylesLegacy} from '../src/style-installer';
 import {parseIfNeeded} from '../src/iframe-helper';
-import installCustomElements from
-  'document-register-element/build/document-register-element.patched';
 
 let iframeCount = 0;