Merge branch 'master' into prefer-destructuring

.eslintrc

@@ -67,9 +67,9 @@
  "amphtml-internal/no-global": 0,
  "amphtml-internal/no-spread": 2,
  "amphtml-internal/no-style-property-setting": 2,
+ "amphtml-internal/prefer-deferred-promise": 1,
  "amphtml-internal/prefer-destructuring": 2,
  "amphtml-internal/query-selector": 2,
- "amphtml-internal/resolve-inside-promise-resolver": 1,
  "amphtml-internal/todo-format": 0,
  "amphtml-internal/unused-private-field": 1,
  "amphtml-internal/vsync": 1,

3p/twitter.js

@@ -17,7 +17,6 @@
 // TODO(malteubl) Move somewhere else since this is not an ad.
 
 import {loadScript} from './3p';
-import {parseUrl} from '../src/url';
 import {setStyles} from '../src/style';
 
 /**
@@ -106,18 +105,15 @@ export function cleanupTweetId_(tweetid) {
  // Handle malformed ids such as
  // https://twitter.com/abc123/status/585110598171631616
  tweetid = tweetid.toLowerCase();
- if (tweetid.indexOf('https://twitter.com/') >= 0) {
- const url = parseUrl(tweetid);
- // pathname = /abc123/status/585110598171631616
- const paths = url.pathname.split('/');
- if (paths[2] == 'status' && paths[3]) {
- return paths[3];
- }
+ let match = tweetid.match(/https:\/\/twitter.com\/[^\/]+\/status\/(\d+)/);
+ if (match) {
+ return match[1];
  }
+
  // 2)
  // Handle malformed ids such as
  // 585110598171631616?ref_src
- const match = tweetid.match(/^(\d+)\?ref.*/);
+ match = tweetid.match(/^(\d+)\?ref.*/);
  if (match) {
  return match[1];
  }

ads/rubicon.js

@@ -14,7 +14,6 @@
  * limitations under the License.
  */
 
-import {getSourceUrl} from '../src/url';
 import {validateData, writeScript} from '../3p/3p';
 
 /**
@@ -39,13 +38,12 @@ export function rubicon(global, data) {
  * @param {!Object} data
  */
 function smartTag(global, data) {
- const pageURL = getSourceUrl(context.location.href);
  /* eslint-disable */
  global.rp_account = data.account;
  global.rp_site = data.site;
  global.rp_zonesize = data.zone + '-' + data.size;
  global.rp_adtype = 'js';
- global.rp_page = pageURL;
+ global.rp_page = context.sourceUrl;
  global.rp_kw = data.kw;
  global.rp_visitor = data.visitor;
  global.rp_inventory = data.inventory;

build-system/eslint-rules/resolve-inside-promise-resolver.js → build-system/eslint-rules/prefer-deferred-promise.js

@@ -16,14 +16,13 @@
 'use strict';
 
 module.exports = function(context) {
- function isResolveCall(node, name) {
- if (node.type !== 'CallExpression') {
+ function isAssignment(node, name) {
+ if (node.type !== 'AssignmentExpression') {
  return false;
  }
 
- const {callee} = node;
- return callee.type === 'Identifier' &&
- callee.name === name;
+ const {right} = node;
+ return right.type === 'Identifier' && right.name === name;
  }
 
  return {
@@ -86,28 +85,36 @@ module.exports = function(context) {
  }
 
  const {name} = resolve;
+ let assigned = false;
 
  if (resolver.type === 'ArrowFunctionExpression' &&
  resolver.expression === true) {
- if (isResolveCall(resolver.body, name)) {
- return;
- }
+ const {body} = resolver;
+ assigned = isAssignment(body, name);
  } else {
  const {body} = resolver.body;
 
  for (let i = 0; i < body.length; i++) {
  const node = body[i];
- if (node.type === 'ExpressionStatement' &&
- isResolveCall(node.expression, name)) {
- return;
+ if (node.type !== 'ExpressionStatement') {
+ continue;
+ }
+
+ const {expression} = node;
+ assigned = isAssignment(expression, name);
+ if (assigned) {
+ break;
  }
  }
  }
 
+ if (!assigned) {
+ return;
+ }
+
  const message = [
- 'Must call the resolve param.',
- 'If you are creating a pending promise to extract a resolve function',
- 'please use Deferred in the src/utils/promise.js module instead.',
+ 'Instead of creating a pending Promise, please use ',
+ 'Deferred in the src/utils/promise.js module.',
  ].join('\n\t');
  context.report(resolver, message);
  },

build-system/tasks/lint.js

@@ -108,10 +108,12 @@ function runLinter(path, stream, options) {
  if (!options.fix) {
  log(colors.yellow('NOTE 1:'),
  'You may be able to automatically fix some of these warnings ' +
- '/ errors by running', colors.cyan('gulp lint --fix') + '.');
+ '/ errors by running',
+ colors.cyan('gulp lint --local-changes --fix'),
+ 'from your local branch.');
  log(colors.yellow('NOTE 2:'),
- 'Since this is a destructive operation (operates on the file',
- 'system), make sure you commit before running the command.');
+ 'Since this is a destructive operation (that edits your files',
+ 'in-place), make sure you commit before running the command.');
  }
  }
  }))
@@ -185,7 +187,8 @@ function lint() {
  enableStrictLinting();
  } else if (!eslintrcChangesInPr() &&
  (process.env.TRAVIS_EVENT_TYPE === 'pull_request' ||
- process.env.LOCAL_PR_CHECK)) {
+ process.env.LOCAL_PR_CHECK ||
+ argv['local-changes'])) {
  const jsFiles = jsFilesInPr();
  if (jsFiles.length == 0) {
  log(colors.green('INFO: ') + 'No JS files in this PR');
@@ -212,6 +215,8 @@ gulp.task(
  {
  options: {
  'watch': ' Watches for changes in files, validates against the linter',
- 'fix': ' Fixes simple lint errors (spacing etc).',
+ 'fix': ' Fixes simple lint errors (spacing etc)',
+ 'local-changes':
+ ' Lints just the changes commited to the local branch',
  },
  });

contributing/DEVELOPING.md

@@ -47,7 +47,8 @@ The Quick Start Guide's [One-time setup](getting-started-quick.md#one-time-setu
 | `gulp lint` | Validates against the ESLint linter. |
 | `gulp lint --watch` | Watches for changes in files, and validates against the ESLint linter. |
 | `gulp lint --fix` | Fixes simple lint warnings/errors automatically. |
-| `gulp lint --files=<files-path-glob>` | Lints just the files provided. |
+| `gulp lint --files=<files-path-glob>` | Lints just the files provided. Can be used with `--fix`. |
+| `gulp lint --local-changes` | Lints just the changes commited to the local branch. Can be used with `--fix`. |
 | `gulp build` | Builds the AMP library. |
 | `gulp build --extensions=<amp-foo,amp-bar>` | Builds the AMP library, with only the listed extensions.
 | `gulp build --extensions=minimal_set` | Builds the AMP library, with only the extensions needed to load `article.amp.html`.

src/utils/promise.js

@@ -14,54 +14,25 @@
  * limitations under the License.
  */
 
-/**
- * Returns a promise which resolves if a threshold amount of the given promises
- * resolve, and rejects otherwise.
- * @param {!Array<!Promise>} promises The array of promises to test.
- * @param {number} count The number of promises that must resolve for the
- * returned promise to resolve.
- * @return {!Promise} A promise that resolves if any of the given promises
- * resolve, and which rejects otherwise.
- */
-export function some(promises, count = 1) {
- return new Promise((resolve, reject) => {
- count = Math.max(count, 0);
- const extra = promises.length - count;
- if (extra < 0) {
- reject(new Error('not enough promises to resolve'));
- }
- if (promises.length == 0) {
- resolve([]);
- }
- const values = [];
- const reasons = [];
-
- const onFulfilled = value => {
- if (values.length < count) {
- values.push(value);
- }
- if (values.length == count) {
- resolve(values);
- }
- };
- const onRejected = reason => {
- if (reasons.length <= extra) {
- reasons.push(reason);
- }
- if (reasons.length > extra) {
- reject(reasons);
- }
- };
- for (let i = 0; i < promises.length; i++) {
- Promise.resolve(promises[i]).then(onFulfilled, onRejected);
- }
- });
-}
-
 /**
  * Returns a Deferred struct, which holds a pending promise and its associated
  * resolve and reject functions.
  *
+ * This is preferred instead of creating a Promise instance to extract the
+ * resolve/reject functions yourself:
+ *
+ * ```
+ * // Avoid doing
+ * let resolve;
+ * const promise = new Promise(res => {
+ * resolve = res;
+ * });
+ *
+ * // Good
+ * const deferred = new Deferred();
+ * const { promise, resolve } = deferred;
+ * ```
+ *
  * @template T
  */
 export class Deferred {
@@ -102,6 +73,50 @@ export function tryResolve(fn) {
  });
 }
 
+/**
+ * Returns a promise which resolves if a threshold amount of the given promises
+ * resolve, and rejects otherwise.
+ * @param {!Array<!Promise>} promises The array of promises to test.
+ * @param {number} count The number of promises that must resolve for the
+ * returned promise to resolve.
+ * @return {!Promise} A promise that resolves if any of the given promises
+ * resolve, and which rejects otherwise.
+ */
+export function some(promises, count = 1) {
+ return new Promise((resolve, reject) => {
+ count = Math.max(count, 0);
+ const extra = promises.length - count;
+ if (extra < 0) {
+ reject(new Error('not enough promises to resolve'));
+ }
+ if (promises.length == 0) {
+ resolve([]);
+ }
+ const values = [];
+ const reasons = [];
+
+ const onFulfilled = value => {
+ if (values.length < count) {
+ values.push(value);
+ }
+ if (values.length == count) {
+ resolve(values);
+ }
+ };
+ const onRejected = reason => {
+ if (reasons.length <= extra) {
+ reasons.push(reason);
+ }
+ if (reasons.length > extra) {
+ reject(reasons);
+ }
+ };
+ for (let i = 0; i < promises.length; i++) {
+ Promise.resolve(promises[i]).then(onFulfilled, onRejected);
+ }
+ });
+}
+
 /**
  * Resolves with the result of the last promise added.
  * @implements {IThenable}