Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code-cleanup of splittable fork. #16846

Merged
merged 1 commit into from
Jul 18, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 0 additions & 15 deletions build-system/base.js

This file was deleted.

1 change: 1 addition & 0 deletions build-system/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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/**/*.*',
Expand Down
120 changes: 3 additions & 117 deletions build-system/get-dep-graph.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand All @@ -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) {
Expand All @@ -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}`;
Expand Down Expand Up @@ -287,7 +260,6 @@ exports.getGraph = function(entryModules, config) {
},
},
packages: {},
browserMask: {},
};

TYPES_VALUES.forEach(type => {
Expand All @@ -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.
Expand All @@ -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) {
Expand All @@ -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;
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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);
}
4 changes: 4 additions & 0 deletions build-system/tasks/presubmit-checks.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
},
Expand Down
7 changes: 6 additions & 1 deletion build-system/tasks/update-packages.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
*/
Expand Down
2 changes: 1 addition & 1 deletion src/polyfills.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
4 changes: 2 additions & 2 deletions src/service/extensions-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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_';
Expand Down
2 changes: 0 additions & 2 deletions test/manual/amp-single-pass-compilation.html
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions testing/describes.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 = ' ';
Expand Down
4 changes: 2 additions & 2 deletions testing/iframe.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down