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

Roll forward CC upgrade with fixes (#18552) #18609

Merged
merged 10 commits into from
Oct 10, 2018
Merged
Show file tree
Hide file tree
Changes from 4 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
11 changes: 7 additions & 4 deletions ads/adventive.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,10 +139,13 @@ function getUrl(context, data, ns) {
* representation of the url search query.
*/
function reduceSearch(ns, placementId, click, referrer) {
const isRecipeLoaded = hasOwn(ns.args, 'placementId'),
isRecipeStale = !isRecipeLoaded ? true :
(Date.now() - ns.args[placementId].requestTime) > (60 * cacheTime),
needsRequest = !isRecipeLoaded || isRecipeStale;
const isRecipeLoaded = hasOwn(ns.args, 'placementId');
let isRecipeStale = true;
if (isRecipeLoaded) {
const info = ns.args[placementId];
isRecipeStale = (Date.now() - info['requestTime']) > (60 * cacheTime);
}
const needsRequest = !isRecipeLoaded || isRecipeStale;
kristoferbaxter marked this conversation as resolved.
Show resolved Hide resolved

return !needsRequest ? null : dict({
'click': click,
Expand Down
8 changes: 7 additions & 1 deletion build-system/get-dep-graph.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,16 @@ exports.getFlags = function(config) {
language_in: 'ES6',
language_out: config.language_out || 'ES5',
module_output_path_prefix: config.writeTo || 'out/',
module_resolution: 'NODE',
externs: config.externs,
define: config.define,
// Turn off warning for "Unknown @define" since we use define to pass
// args such as FORTESTING to our runner.
jscomp_off: ['unknownDefines'],
// checkVars: Demote "variable foo is undeclared" errors.
// moduleLoad: Demote "module not found" errors to ignore missing files
// in type declarations in the swg.js bundle.
jscomp_warning: ['checkVars', 'moduleLoad'],
Copy link
Member

@erwinmombay erwinmombay Oct 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did we need this for both compile and check-type tasks? was wondering if we could make it conditional

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we be consistent between both?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@choumx ideally. i just remembered the scenario you ran into originally with multi-pass, where the compilation worked but check-types failed. was that more an inconsistency with another option?

jscomp_error: [
'checkTypes',
'accessControls',
Expand Down Expand Up @@ -206,7 +211,8 @@ exports.getBundleFlags = function(g) {
throw new Error('Expect to build more than one bundle.');
}
});
flagsArray.push('--js_module_root', `${g.tmp}/node_modules/`);
// Do _not_ include 'node_modules/' in js_module_root with 'NODE' resolution
kristoferbaxter marked this conversation as resolved.
Show resolved Hide resolved
// or bad things will happen (#18600).
flagsArray.push('--js_module_root', `${g.tmp}/`);
return flagsArray;
};
Expand Down
Binary file modified build-system/runner/dist/runner.jar
Binary file not shown.
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,6 @@ protected AmpCommandLineRunner(String[] args) {
options.setDevirtualizePrototypeMethods(true);
options.setExtractPrototypeMemberDeclarations(true);
options.setSmartNameRemoval(true);
options.optimizeParameters = true;
options.optimizeReturns = true;
options.optimizeCalls = true;
if (single_file_compilation) {
options.renamePrefixNamespace = "_";
Expand Down
12 changes: 7 additions & 5 deletions build-system/runner/src/org/ampproject/AmpPass.java
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ private void inlineAmpExtensionCall(Node n, Node expr) {
newcall.putBooleanProp(Node.FREE_CALL, true);
newcall.addChildToBack(arg1);
expr.replaceChild(n, newcall);
compiler.reportCodeChange();
compiler.reportChangeToEnclosingScope(expr);
}

private Node getAmpExtensionCallback(Node n) {
Expand Down Expand Up @@ -226,7 +226,7 @@ private void maybeReplaceRValueInVar(Node n, Map<String, Node> map) {
for (Map.Entry<String, Node> mapping : map.entrySet()) {
if (varNode.getString() == mapping.getKey()) {
varNode.replaceChild(varNode.getFirstChild(), mapping.getValue());
compiler.reportCodeChange();
compiler.reportChangeToEnclosingScope(varNode);
return;
}
}
Expand Down Expand Up @@ -276,17 +276,19 @@ private void replaceWithBooleanExpression(boolean bool, Node n, Node parent) {
Node booleanNode = bool ? IR.trueNode() : IR.falseNode();
booleanNode.useSourceInfoIfMissingFrom(n);
parent.replaceChild(n, booleanNode);
compiler.reportCodeChange();
compiler.reportChangeToEnclosingScope(parent);
}

private void removeExpression(Node n, Node parent) {
Node scope = parent;
if (parent.isExprResult()) {
Node grandparent = parent.getParent();
grandparent.removeChild(parent);
scope = grandparent;
} else {
parent.removeChild(n);
}
compiler.reportCodeChange();
compiler.reportChangeToEnclosingScope(scope);
}

private void maybeEliminateCallExceptFirstParam(Node call, Node parent) {
Expand All @@ -306,7 +308,7 @@ private void maybeEliminateCallExceptFirstParam(Node call, Node parent) {

firstArg.detachFromParent();
parent.replaceChild(call, firstArg);
compiler.reportCodeChange();
compiler.reportChangeToEnclosingScope(parent);
}

/**
Expand Down
10 changes: 5 additions & 5 deletions build-system/runner/test/org/ampproject/AmpPassTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,15 @@
import com.google.common.collect.ImmutableSet;
import com.google.javascript.jscomp.Compiler;
import com.google.javascript.jscomp.CompilerPass;
import com.google.javascript.jscomp.Es6CompilerTestCase;
import com.google.javascript.jscomp.CompilerTestCase;
import com.google.javascript.rhino.IR;
import com.google.javascript.rhino.Node;


/**
* Tests {@link AmpPass}.
*/
public class AmpPassTest extends Es6CompilerTestCase {
public class AmpPassTest extends CompilerTestCase {

ImmutableMap<String, Set<String>> suffixTypes = ImmutableMap.of(
"module$src$log.dev",
Expand Down Expand Up @@ -305,7 +305,7 @@ public void testGetModePreserve() throws Exception {
}

public void testOptimizeGetModeFunction() throws Exception {
testEs6(
test(
LINE_JOINER.join(
"(function() {",
"const IS_DEV = true;",
Expand All @@ -321,7 +321,7 @@ public void testOptimizeGetModeFunction() throws Exception {
}

public void testRemoveAmpAddExtensionCallWithExplicitContext() throws Exception {
testEs6(
test(
LINE_JOINER.join(
"var a = 'hello';",
"self.AMP.extension('hello', '0.1', function(AMP) {",
Expand All @@ -339,7 +339,7 @@ public void testRemoveAmpAddExtensionCallWithExplicitContext() throws Exception
}

public void testRemoveAmpAddExtensionCallWithNoContext() throws Exception {
testEs6(
test(
LINE_JOINER.join(
"var a = 'hello';",
"AMP.extension('hello', '0.1', function(AMP) {",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,15 @@
import com.google.common.collect.ImmutableMap;
import com.google.javascript.rhino.IR;
import com.google.javascript.rhino.Node;
import com.google.common.collect.ImmutableSet;
import com.google.javascript.jscomp.Compiler;
import com.google.javascript.jscomp.CompilerPass;
import com.google.javascript.jscomp.Es6CompilerTestCase;
import com.google.javascript.jscomp.CompilerTestCase;


/**
* Tests {@link AmpPass}.
*/
public class AmpPassTestEnvTest extends Es6CompilerTestCase {
public class AmpPassTestEnvTest extends CompilerTestCase {

ImmutableMap<String, Set<String>> suffixTypes = ImmutableMap.of();
ImmutableMap<String, Node> assignmentReplacements = ImmutableMap.of(
Expand Down Expand Up @@ -97,7 +96,7 @@ public void testGetModeMinifiedPropertyReplacementInTestingEnv() throws Exceptio
}

public void testOptimizeGetModeFunction() throws Exception {
testEs6(
test(
LINE_JOINER.join(
"(function() {",
"const IS_DEV = true;",
Expand Down
2 changes: 1 addition & 1 deletion build-system/tasks/bundle-size.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const log = require('fancy-log');
const {getStdout} = require('../exec');

const runtimeFile = './dist/v0.js';
const maxSize = '81.8KB'; // Only use 0.1 KB of precision (no hundredths digit)
const maxSize = '82.2KB'; // Only use 0.1 KB of precision (no hundredths digit)

const {green, red, cyan, yellow} = colors;

Expand Down
24 changes: 14 additions & 10 deletions build-system/tasks/compile.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ exports.closureCompile = function(entryModuleFilename, outputDir,
next();
resolve();
}, function(e) {
console./* OK*/error(colors.red('Compilation error:', e.message));
console./*OK*/error(colors.red('Compilation error:'), e.message);
process.exit(1);
});
}
Expand Down Expand Up @@ -89,14 +89,13 @@ exports.cleanupBuildDir = cleanupBuildDir;
function formatClosureCompilerError(message) {
const javaInvocationLine = /Command failed:[^]*--js_output_file=\".*?\"\n/;
message = message.replace(javaInvocationLine, '');
message = highlight(message, {ignoreIllegals: true}); // never throws
message = highlight(message, {ignoreIllegals: true});
message = message.replace(/WARNING/g, colors.yellow('WARNING'));
message = message.replace(/ERROR/g, colors.red('ERROR'));
return message;
}

function compile(entryModuleFilenames, outputDir,
outputFilename, options) {
function compile(entryModuleFilenames, outputDir, outputFilename, options) {
const hideWarningsFor = [
'third_party/caja/',
'third_party/closure-library/sha384-generated.js',
Expand All @@ -114,10 +113,8 @@ function compile(entryModuleFilenames, outputDir,
// Generated code.
'extensions/amp-access/0.1/access-expr-impl.js',
];

const baseExterns = [
'build-system/amp.extern.js',
'third_party/closure-compiler/externs/performance_observer.js',
'third_party/closure-compiler/externs/web_animations.js',
'third_party/moment/moment.extern.js',
'third_party/react-externs/externs.js',
Expand All @@ -142,9 +139,11 @@ function compile(entryModuleFilenames, outputDir,
define.push('ESM_BUILD=true');
}

console/*OK*/.assert(typeof entryModuleFilenames == 'string');
const entryModule = entryModuleFilenames;
// TODO(@cramforce): Run the post processing step
return singlePassCompile(
entryModuleFilenames,
entryModule,
compilationOptions
).then(() => {
return new Promise((resolve, reject) => {
Expand Down Expand Up @@ -252,7 +251,7 @@ function compile(entryModuleFilenames, outputDir,
'third_party/webcomponentsjs/ShadowCSS.js',
'third_party/rrule/rrule.js',
'third_party/react-dates/bundle.js',
'node_modules/dompurify/dist/purify.cjs.js',
'node_modules/dompurify/dist/purify.es.js',
kristoferbaxter marked this conversation as resolved.
Show resolved Hide resolved
'node_modules/promise-pjs/promise.js',
'node_modules/set-dom/src/**/*.js',
'node_modules/web-animations-js/web-animations.install.js',
Expand Down Expand Up @@ -352,12 +351,14 @@ function compile(entryModuleFilenames, outputDir,
rewrite_polyfills: false,
externs,
js_module_root: [
'node_modules/',
// Do _not_ include 'node_modules/' in js_module_root with 'NODE'
// resolution or bad things will happen (#18600).
'build/patched-module/',
'build/fake-module/',
'build/fake-polyfills/',
],
entry_point: entryModuleFilenames,
module_resolution: 'NODE',
process_common_js_modules: true,
// This strips all files from the input set that aren't explicitly
// required.
Expand All @@ -367,12 +368,15 @@ function compile(entryModuleFilenames, outputDir,
source_map_location_mapping:
'|' + sourceMapBase,
warning_level: 'DEFAULT',
jscomp_error: [],
// moduleLoad: Demote "module not found" errors to ignore missing files
// in type declarations in the swg.js bundle.
jscomp_warning: ['moduleLoad'],
// Turn off warning for "Unknown @define" since we use define to pass
// args such as FORTESTING to our runner.
jscomp_off: ['unknownDefines'],
define,
hide_warnings_for: hideWarningsFor,
jscomp_error: [],
},
};

Expand Down
9 changes: 7 additions & 2 deletions build-system/tasks/dep-check.js
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,13 @@ function getGraph(entryModule) {

// TODO(erwinm): Try and work this in with `gulp build` so that
// we're not running browserify twice on travis.
const bundler = browserify(entryModule, {debug: true, deps: true})
.transform(babelify, {compact: false});
const bundler = browserify(entryModule, {debug: true})
.transform(babelify, {
compact: false,
// Transform files in node_modules since deps use ES6 export.
kristoferbaxter marked this conversation as resolved.
Show resolved Hide resolved
// https://github.com/babel/babelify#why-arent-files-in-node_modules-being-transformed
global: true,
});

bundler.pipeline.get('deps').push(through.obj(function(row, enc, next) {
module.deps.push({
Expand Down
10 changes: 9 additions & 1 deletion build-system/tasks/karma.conf.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,15 @@ module.exports = {
debug: true,
basedir: __dirname + '/../../',
transform: [
['babelify', {compact: false, sourceMapsAbsolute: true}],
[
'babelify', {
compact: false,
// Transform files in node_modules since deps use ES6 export.
// https://github.com/babel/babelify#why-arent-files-in-node_modules-being-transformed
global: true,
sourceMapsAbsolute: true,
},
],
],
// Prevent "cannot find module" errors on Travis. See #14166.
bundleDelay: process.env.TRAVIS ? 5000 : 1200,
Expand Down
3 changes: 3 additions & 0 deletions build-system/tasks/runtime-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -501,6 +501,9 @@ function runTests() {
c.browserify.transform = [
['babelify', {
compact: false,
// Transform files in node_modules since deps use ES6 export.
// https://github.com/babel/babelify#why-arent-files-in-node_modules-being-transformed
global: true,
plugins: [
['babel-plugin-istanbul', {
exclude: [
Expand Down
7 changes: 5 additions & 2 deletions build-system/tasks/update-packages.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ function patchWebAnimations() {
'node_modules/web-animations-js/' +
'web-animations.min.js').toString();
// Wrap the contents inside the install function.
file = 'exports.installWebAnimations = function(window) {\n' +
file = 'export function installWebAnimations(window) {\n' +
'var document = window.document;\n' +
file.replace(/requestAnimationFrame/g, function(a, b) {
if (file.charAt(b - 1) == '.') {
Expand Down Expand Up @@ -80,12 +80,14 @@ function patchRegisterElement() {
file = fs.readFileSync(
'node_modules/document-register-element/build/' +
'document-register-element.node.js').toString();

// Eliminate the immediate side effect.
if (!/installCustomElements\(global\);/.test(file)) {
throw new Error('Expected "installCustomElements(global);" ' +
'to appear in document-register-element');
}
file = file.replace('installCustomElements(global);', '');

// Closure Compiler does not generate a `default` property even though
// to interop CommonJS and ES6 modules. This is the same issue typescript
// ran into here https://github.com/Microsoft/TypeScript/issues/2719
Expand All @@ -94,7 +96,8 @@ function patchRegisterElement() {
'to appear in document-register-element');
}
file = file.replace('module.exports = installCustomElements;',
'exports.installCustomElements = installCustomElements;');
'export {installCustomElements};');

writeIfUpdated(patchedName, file);
}

Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-analytics/0.1/iframe-transport.js
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ export class IframeTransport {
(entry['name'] == 'cross-origin-descendant') &&
entry.attribution) {
entry.attribution.forEach(attrib => {
if (this.frameUrl_ == attrib.containerSrc &&
if (this.frameUrl_ == attrib['containerSrc'] &&
++this.numLongTasks_ % LONG_TASK_REPORTING_THRESHOLD == 0) {
user().error(TAG_, `Long Task: Vendor: "${this.type_}"`);
}
Expand Down
5 changes: 3 additions & 2 deletions extensions/amp-mustache/0.1/amp-mustache.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ export class AmpMustache extends AMP.BaseTemplate {
container.appendChild(content);
/** @private @const {string} */
this.template_ = container./*OK*/innerHTML;
mustacheParse(this.template_);
mustacheParse(this.template_, /* tags */ undefined);
}

/** @override */
Expand All @@ -91,7 +91,8 @@ export class AmpMustache extends AMP.BaseTemplate {
if (typeof data === 'object') {
mustacheData = Object.assign({}, data, this.nestedTemplates_);
}
html = mustacheRender(this.template_, mustacheData);
html = mustacheRender(this.template_, mustacheData,
/* partials */ undefined);
}
return this.serializeHtml_(html);
}
Expand Down
Loading