diff --git a/README.md b/README.md index 9d3877c22..1c984c40c 100644 --- a/README.md +++ b/README.md @@ -72,7 +72,7 @@ This codebase adheres to the [Googe Javascript Styleguide](https://google.github npm run lint # run ESLint with `--fix` option to automatically fix errors (if possible) -npm run lint-fix +npm run lint:fix ``` ## Contributing diff --git a/package.json b/package.json index 508fff0f0..12a2a006c 100644 --- a/package.json +++ b/package.json @@ -7,7 +7,7 @@ "test:node": "jasmine --config=jasmine.json", "test:browser": "karma start --single-run --browsers ChromeHeadless karma.conf.js", "lint": "eslint \"**/*.js\" --ignore-path .gitignore", - "lint-fix": "eslint \"**/*.js\" --fix --ignore-path .gitignore" + "lint:fix": "eslint \"**/*.js\" --fix --ignore-path .gitignore" }, "devDependencies": { "@ampproject/rollup-plugin-closure-compiler": "0.8.3", diff --git a/packages/cli/index.js b/packages/cli/index.js index 5822dafe1..74228ec83 100644 --- a/packages/cli/index.js +++ b/packages/cli/index.js @@ -17,15 +17,14 @@ 'use strict'; const Cli = require('./lib/cli'); -const Logger = require('./lib/util/Logger'); +const {log} = require('amp-toolbox-core'); module.exports = () => { - const logger = new Logger(); - const cli = new Cli(logger); + const cli = new Cli(log); const args = process.argv.slice(2); cli.run(args) .catch((err) => { - logger.error(err.message); + log.error(err.message); process.exit(1); }); }; diff --git a/packages/cli/lib/cli.js b/packages/cli/lib/cli.js index 2f621e62b..3db8119fe 100644 --- a/packages/cli/lib/cli.js +++ b/packages/cli/lib/cli.js @@ -17,11 +17,11 @@ 'use strict'; const minimist = require('minimist'); -const Logger = require('./util/Logger'); +const {log} = require('amp-toolbox-core'); class Cli { - constructor(logger = new Logger()) { - this.logger = logger; + constructor(logger=log) { + this.logger_ = logger; } run(args) { @@ -30,11 +30,11 @@ class Cli { switch (command) { case 'help': - return require('./cmds/help')(args, this.logger); + return require('./cmds/help')(args, this.logger_); case 'version': - return require('./cmds/version')(args, this.logger); + return require('./cmds/version')(args, this.logger_); case 'update-cache': - return require('./cmds/updateCache')(args, this.logger); + return require('./cmds/updateCache')(args, this.logger_); default: return Promise.reject(new Error(`"${command}" is not a valid command!`)); } diff --git a/packages/cli/lib/cmds/updateCache.js b/packages/cli/lib/cmds/updateCache.js index 7f3db43e0..2ab9aa529 100644 --- a/packages/cli/lib/cmds/updateCache.js +++ b/packages/cli/lib/cmds/updateCache.js @@ -36,14 +36,14 @@ function updateCaches_(privateKey, url, logger) { } function updateCache_(cacheUpdateUrlInfo, logger) { - const tag = cacheUpdateUrlInfo.cacheName; - logger.info(`Invalidating ${cacheUpdateUrlInfo.cacheName}`, tag); - logger.info(`Using Invalidation URL: ${cacheUpdateUrlInfo.updateCacheUrl}`, tag); + logger = logger.tag(cacheUpdateUrlInfo.cacheName); + logger.info(`Invalidating ${cacheUpdateUrlInfo.cacheName}`); + logger.info(`Using Invalidation URL: ${cacheUpdateUrlInfo.updateCacheUrl}`); fetch(cacheUpdateUrlInfo.updateCacheUrl) .then((response) => { if (response.status === 200) { - logger.success(`${cacheUpdateUrlInfo.cacheName} Updated`, tag); + logger.success(`${cacheUpdateUrlInfo.cacheName} Updated`); return; } return response.text() @@ -52,18 +52,18 @@ function updateCache_(cacheUpdateUrlInfo, logger) { if (match) { logger.error( `Error Invalidating Cache URL. Received response code "${response.status}" ` + - `with message: "${match[1]}"`, tag + `with message: "${match[1]}"` ); } else { logger.error( `Error Invalidating Cache URL. Received response code "${response.status}"` + - 'with an unknown error', tag + 'with an unknown error' ); } }); }) .catch((e) => { - logger.warn(`Error connecting to the AMP Cache with message: "${e.message}"`, tag); + logger.warn(`Error connecting to the AMP Cache with message: "${e.message}"`); }); } diff --git a/packages/cli/lib/util/Logger.js b/packages/cli/lib/util/Logger.js deleted file mode 100644 index f81023595..000000000 --- a/packages/cli/lib/util/Logger.js +++ /dev/null @@ -1,55 +0,0 @@ -/** - * Copyright 2017 The AMP HTML Authors. All Rights Reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS-IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -'use strict'; - -const FORMAT_RED = '\x1b[31m'; // Switches console color to red. -const FORMAT_GREEN = '\x1b[32m'; // Switches console color to green. -const FORMAT_YELLOW = '\x1b[33m'; // Switches console color to yello. -const FORMAT_RESET = '\x1b[0m'; // Resets the console color. -const FORMAT_HIGHLIGHT = '\x1b[1m'; // Highlights the console color. - -const TAG_ERROR = `${FORMAT_RED}ERROR!${FORMAT_RESET}`; -const TAG_SUCCESS = `${FORMAT_GREEN}SUCCESS!${FORMAT_RESET}`; -const TAG_WARNING = `${FORMAT_YELLOW}WARNING!${FORMAT_RESET}`; - -class Logger { - error(args, tag = null) { - console.error(this.formatTag_(tag), TAG_ERROR, args); - } - - warn(args, tag = null) { - console.log(this.formatTag_(tag), TAG_WARNING, args); - } - - success(args, tag = null) { - console.log(this.formatTag_(tag), TAG_SUCCESS, args); - } - - info(args, tag = null) { - console.log(this.formatTag_(tag), args); - } - - formatTag_(tag) { - if (!tag) { - return ''; - } - - return `${FORMAT_HIGHLIGHT}[${tag}]:${FORMAT_RESET}`; - } -} - -module.exports = Logger; diff --git a/packages/core/index.js b/packages/core/index.js index f1c5a923a..a4cc1fa9d 100644 --- a/packages/core/index.js +++ b/packages/core/index.js @@ -18,5 +18,6 @@ module.exports = { MaxAge: require('./lib/MaxAge.js'), OneBehindFetch: require('./lib/OneBehindFetch.js'), + log: require('./lib/Log.js'), }; diff --git a/packages/core/lib/Log.js b/packages/core/lib/Log.js new file mode 100644 index 000000000..f18734a9a --- /dev/null +++ b/packages/core/lib/Log.js @@ -0,0 +1,70 @@ +class Log { + constructor(tag='', verbose=false, output=console) { + this.tag_ = tag; + this.verbose_ = verbose; + this.prefix_ = this.inverse_(tag); + this.output_ = output; + } + + debug(message, ...args) { + if (!this.verbose_) { + return; + } + this.log_(this.output_.log, this.dim_(message), args); + } + + info(message, ...args) { + this.log_(this.output_.log, message, ...args); + } + + warn(message, ...args) { + this._log(this.output_.warn, this.yellow_('WARNING ' + message), args); + } + + error(message, ...args) { + this.log_(this.output_.error, '\n'); + this.log_(this.output_.error, this.red_('ERROR ' + message), args); + this.log_(this.output_.error, '\n'); + } + + verbose(isVerbose=true) { + this.verbose_ = Boolean.valueOf(isVerbose); + } + + tag(newTag) { + if (this.tag_) { + newTag = this.tag_ + ' ' + newTag; + } + return new Log(newTag, this.verbose_, this.output_); + } + + log_(fn, message, args) { + if (this.prefix_) { + message = this.prefix_ + ' ' + message; + } + if (args) { + fn(...[message].concat(args)); + } else { + fn(message); + } + } + + inverse_(string) { + return `\x1b[7m${string}\x1b[0m`; + } + + dim_(string) { + return `\x1b[36m${string}\x1b[0m`; + } + + yellow_(string) { + return `\x1b[33m${string}\x1b[0m`; + } + + red_(string) { + return `\x1b[31m${string}\x1b[0m`; + } +} + +module.exports = new Log(); + diff --git a/packages/optimizer/README.md b/packages/optimizer/README.md index 0d71e59d8..aec8706ac 100644 --- a/packages/optimizer/README.md +++ b/packages/optimizer/README.md @@ -220,6 +220,26 @@ A few notable changes are: ## Best Practices +### Debugging + +Enable `verbose` mode to find out why the AMP boilerplate is not being removed. + +``` +// globally +ampOptimizer.setConfig({ + verbose: true +}); +ampOptimizer.transformHtml(originalHtml, { + ampUrl: 'canonical.amp.html' +}) + +// per transformation +ampOptimizer.transformHtml(originalHtml, { + ampUrl: 'canonical.amp.html', + verbose: true +}) +``` + ### Transform AMP pages at build time if possible Applying the transformations to an AMP file consumes additional server diff --git a/packages/optimizer/index.js b/packages/optimizer/index.js index c6a38f662..c739fea78 100644 --- a/packages/optimizer/index.js +++ b/packages/optimizer/index.js @@ -17,26 +17,4 @@ const DomTransformer = require('./lib/DomTransformer.js'); -const defaultConfig = { - transformers: [ - // Adds a link to the valid AMP version - 'AddAmpLink', - // Applies server-side-rendering optimizations - 'ServerSideRendering', - // Removes ⚡ or 'amp' from the html tag - 'RemoveAmpAttribute', - // Removes the boilerplate - // needs to run after ServerSideRendering - 'AmpBoilerplateTransformer', - // Optimizes script import order - // needs to run after ServerSideRendering - 'ReorderHeadTransformer', - // needs to run after ReorderHeadTransformer - 'RewriteAmpUrls', - 'GoogleFontsPreconnect', - 'PruneDuplicateResourceHints', - 'AddBlurryImagePlaceholders', - ], -}; - -module.exports = new DomTransformer(defaultConfig); +module.exports = new DomTransformer(); diff --git a/packages/optimizer/lib/DomTransformer.js b/packages/optimizer/lib/DomTransformer.js index 5d85f9ad4..1f3905d3e 100644 --- a/packages/optimizer/lib/DomTransformer.js +++ b/packages/optimizer/lib/DomTransformer.js @@ -17,6 +17,50 @@ const path = require('path'); const treeParser = require('./TreeParser.js'); +const log = require('./log.js'); + +/** + * AMP Optimizer Configuration only applying AMP validity perserving transformations. + */ +const TRANSFORMATIONS_VALID_AMP = [ + // Optimizes script import order + // needs to run after ServerSideRendering + 'ReorderHeadTransformer', + // needs to run after ReorderHeadTransformer + 'RewriteAmpUrls', + 'GoogleFontsPreconnect', + 'PruneDuplicateResourceHints', +]; + +/** + * AMP Optimizer Configuration applying all available AMP optimizations including Server-Side_Rendering. + */ +const TRANSFORMATIONS_ALL = [ + // Adds a link to the valid AMP version + 'AddAmpLink', + // Applies server-side-rendering optimizations + 'ServerSideRendering', + // Removes ⚡ or 'amp' from the html tag + 'RemoveAmpAttribute', + // Removes the boilerplate + // needs to run after ServerSideRendering + 'AmpBoilerplateTransformer', + // Optimizes script import order + // needs to run after ServerSideRendering + 'ReorderHeadTransformer', + // needs to run after ReorderHeadTransformer + 'RewriteAmpUrls', + 'GoogleFontsPreconnect', + 'PruneDuplicateResourceHints', + 'AddBlurryImagePlaceholders', +]; + +const DEFAULT_CONFIG = { + verbose: false, + validAmp: false, + transformers: TRANSFORMATIONS_ALL, +}; + /** * Applies a set of transformations to a DOM tree. @@ -27,8 +71,9 @@ class DomTransformer { * @param {Object} config - The config. * @param {Array.} config.transformers - a list of transformers to be applied. */ - constructor(config) { + constructor(config=DEFAULT_CONFIG) { this.setConfig(config); + this.log_ = log; } /** @@ -37,7 +82,6 @@ class DomTransformer { * @param {Object} params - a dictionary containing transformer specific parameters. */ transformHtml(html, params) { - params = params || {}; const tree = treeParser.parse(html); return this.transformTree(tree, params) .then(() => treeParser.serialize(tree)); @@ -49,28 +93,47 @@ class DomTransformer { * @param {Object} params - a dictionary containing transformer specific parameters. */ transformTree(tree, params) { + params = params || {}; + log.verbose(params.verbose || false); const sequence = (promise, transformer) => { return promise.then(() => { - const result = Promise.resolve(transformer.transform(tree, params)); - return result; + // not all transformers return a promise + return Promise.resolve(transformer.transform(tree, params, this.log_)); }); }; - return this._transformers.reduce(sequence, Promise.resolve()); + return this.transformers_.reduce(sequence, Promise.resolve()); } /** * Set the config. * @param {Object} config - The config. - * @param {Array.} config.transformers - a list of transformers to be applied. + * @param {boolean} config.verbose - true if verbose mode should be enabled [default: false]. + * @param {boolean} config.validAmp - true if AMP pages should stay valid [default: false]. + * @param {Array.} config.transformers - a list of transformers to be applied [default: all available transformers]. */ setConfig(config) { - this._transformers = config.transformers.map((transformer) => { + log.verbose(config.verbose); + this.initTransformers_(config); + } + + initTransformers_(config) { + this.transformers_ = this.getTransformersFromConfig_(config).map((transformer) => { if (typeof transformer === 'string') { return require(path.join(__dirname, 'transformers', transformer + '.js')); } return transformer; }); } + + getTransformersFromConfig_(config) { + if (config.transformers) { + return config.transformers; + } + if (config.validAmp) { + return TRANSFORMATIONS_VALID_AMP; + } + return TRANSFORMATIONS_ALL; + } } module.exports = DomTransformer; diff --git a/packages/optimizer/lib/log.js b/packages/optimizer/lib/log.js new file mode 100644 index 000000000..c76604d3f --- /dev/null +++ b/packages/optimizer/lib/log.js @@ -0,0 +1,22 @@ +/** + * Copyright 2017 The AMP HTML Authors. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS-IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +'use strict'; + +const {log} = require('amp-toolbox-core'); + +module.exports = log.tag('AMP Optimizer'); + diff --git a/packages/optimizer/lib/transformers/AddAmpLink.js b/packages/optimizer/lib/transformers/AddAmpLink.js index bca568b39..eadb5c175 100644 --- a/packages/optimizer/lib/transformers/AddAmpLink.js +++ b/packages/optimizer/lib/transformers/AddAmpLink.js @@ -18,6 +18,10 @@ /** * AddAmpLink - adds a reference to the valid AMP * version of this document. + * + * This transformer supports the following parameter(s): + * + * * `ampUrl`: specifying an URL pointing to the valid AMP version of this document. */ class AddAmpLink { transform(tree, params) { diff --git a/packages/optimizer/lib/transformers/AddBlurryImagePlaceholders.js b/packages/optimizer/lib/transformers/AddBlurryImagePlaceholders.js index 9a171a787..aeab5a3a4 100644 --- a/packages/optimizer/lib/transformers/AddBlurryImagePlaceholders.js +++ b/packages/optimizer/lib/transformers/AddBlurryImagePlaceholders.js @@ -18,6 +18,7 @@ const {join, resolve} = require('path'); const {URL} = require('url'); const jimp = require('jimp'); const {skipNodeAndChildren} = require('../HtmlDomHelper'); +const {log} = require('../log.js'); const PIXEL_TARGET = 60; const MAX_BLURRED_PLACEHOLDERS = 5; @@ -134,7 +135,7 @@ class AddBlurryImagePlaceholders { return img; }) .catch((err) => { - console.error(`[AddBlurryImagePlaceholders] ${err.message}`); + log.error(`[AddBlurryImagePlaceholders] ${err.message}`); }); } diff --git a/packages/optimizer/lib/transformers/ApplyLayout.js b/packages/optimizer/lib/transformers/ApplyLayout.js index b75f729a1..34892cff9 100644 --- a/packages/optimizer/lib/transformers/ApplyLayout.js +++ b/packages/optimizer/lib/transformers/ApplyLayout.js @@ -15,6 +15,7 @@ */ 'use strict'; +const log = require('../log.js'); const { parseLayout, cssLength, @@ -102,19 +103,22 @@ function maybeAddSizerInto(node, tree, layout, width, height) { module.exports = { applyLayout: function(customElement, tree) { const ampLayout = parseLayout(customElement.attribs.layout); + const widthAttribute = getAttributeOrNull(customElement, 'width'); const inputWidth = cssLength( - getAttributeOrNull(customElement, 'width'), + widthAttribute, /* allow_auto=*/true, /* allow_fluid=*/false); if (!inputWidth.isValid) { + log.debug('Cannot perform SSR: invalid input width\n', widthAttribute); return false; } - const inputHeight = cssLength( - getAttributeOrNull(customElement, 'height'), + const heightAttribute = getAttributeOrNull(customElement, 'height'); + const inputHeight = cssLength(heightAttribute, /* allow_auto=*/true, /* allow_fluid=*/ampLayout === 'fluid' ); if (!inputHeight.isValid) { + log.debug('Cannot perform SSR: invalid input height\n', heightAttribute); return false; } @@ -126,6 +130,7 @@ module.exports = { getAttributeOrNull(customElement, 'sizes'), getAttributeOrNull(customElement, 'heights')); if (!isSupportedLayout(layout)) { + log.debug('Cannot perform SSR: unsupported layout', layout); return false; } diff --git a/packages/optimizer/lib/transformers/GoogleFontsPreconnect.js b/packages/optimizer/lib/transformers/GoogleFontsPreconnect.js index da6ed7923..27e82ea08 100644 --- a/packages/optimizer/lib/transformers/GoogleFontsPreconnect.js +++ b/packages/optimizer/lib/transformers/GoogleFontsPreconnect.js @@ -17,6 +17,7 @@ 'use strict'; const {findMetaViewport} = require('../HtmlDomHelper'); +const log = require('../log.js'); /** * Adds a preconnect instruction to `fonts.gstatic.com` when the Google Fonts CSS @@ -41,11 +42,15 @@ class GoogleFontsPreconnect { if (this.isGoogleFontsLinkNode_(node)) { // Preconnect to fonts.gstatic.com, where the final fonts are downloaded. const linkPreconnect = tree.createElement('link'); - linkPreconnect.attribs.rel = 'preconnect'; + linkPreconnect.attribs.rel = 'dns-prefetch preconnect'; linkPreconnect.attribs.href = 'https://fonts.gstatic.com'; linkPreconnect.attribs.crossorigin = ''; const referenceNode = findMetaViewport(head); head.insertAfter(linkPreconnect, referenceNode); + log.debug( + 'Adding '); // We only need 1 preconnect, so we can skip the remaining elements and return. return; diff --git a/packages/optimizer/lib/transformers/RewriteAmpUrls.js b/packages/optimizer/lib/transformers/RewriteAmpUrls.js index 500185204..09cb5d059 100644 --- a/packages/optimizer/lib/transformers/RewriteAmpUrls.js +++ b/packages/optimizer/lib/transformers/RewriteAmpUrls.js @@ -21,7 +21,7 @@ const {findMetaViewport} = require('../HtmlDomHelper'); /** * RewriteAmpUrls - rewrites AMP runtime URLs. * - * This transformer supports two options: + * This transformer supports two parameters: * * * `ampRuntimeVersion`: specifies a * [specific version](https://github.com/ampproject/amp-toolbox/tree/master/runtime-version") diff --git a/packages/optimizer/lib/transformers/ServerSideRendering.js b/packages/optimizer/lib/transformers/ServerSideRendering.js index b7a357a03..b8f0ecec1 100644 --- a/packages/optimizer/lib/transformers/ServerSideRendering.js +++ b/packages/optimizer/lib/transformers/ServerSideRendering.js @@ -17,6 +17,7 @@ const {isRenderDelayingExtension, isCustomElement} = require('../Extensions.js'); const {applyLayout} = require('./ApplyLayout.js'); +const log = require('../log.js'); class ServerSideRendering { // Determines whether the node |n| has an enclosing ancestor tag @@ -61,6 +62,10 @@ class ServerSideRendering { // the document, we can't remove the boilerplate - they require the // boilerplate. if (node.attribs.heights || node.attribs.media || node.attribs.sizes) { + log.debug( + 'Cannot remove boilerplate as either heights, media are sizes attribute is set:\n', + node.attribs + ); canRemoveBoilerplate = false; } @@ -69,6 +74,7 @@ class ServerSideRendering { // of the amp-experiment script in IsRenderDelayingExtension below. if (node.tagName === 'amp-experiment') { canRemoveBoilerplate = false; + log.debug('Cannot remove boilerplate: amp-experiment'); } // amp-audio requires knowing the dimensions of the browser. Do not @@ -76,6 +82,7 @@ class ServerSideRendering { // document. if (node.tagName === 'amp-audio') { canRemoveBoilerplate = false; + log.debug('Cannot remove boilerplate: amp-audio'); continue; } @@ -83,6 +90,7 @@ class ServerSideRendering { // any unsupported layout, the applyLayout function returns // false and we can't remove the boilerplate. if (!applyLayout(node, tree)) { + log.debug('Cannot remove boilerplate: unsupported layout'); canRemoveBoilerplate = false; continue; } @@ -104,6 +112,7 @@ class ServerSideRendering { continue; } if (isRenderDelayingExtension(node)) { + log.debug('Cannot remove boilerplate: amp-dynamic-css-classes'); canRemoveBoilerplate = false; } } diff --git a/packages/optimizer/spec/helpers/logging.js b/packages/optimizer/spec/helpers/logging.js new file mode 100644 index 000000000..dd035e98d --- /dev/null +++ b/packages/optimizer/spec/helpers/logging.js @@ -0,0 +1,17 @@ +/** + * Copyright 2017 The AMP HTML Authors. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS-IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +require('../../lib/log.js').verbose(); diff --git a/packages/optimizer/spec/transformers/GoogleFontsPreconnect/adds_only_one_preconnect/expected_output.html b/packages/optimizer/spec/transformers/GoogleFontsPreconnect/adds_only_one_preconnect/expected_output.html index 0c7b11c0b..933751a16 100644 --- a/packages/optimizer/spec/transformers/GoogleFontsPreconnect/adds_only_one_preconnect/expected_output.html +++ b/packages/optimizer/spec/transformers/GoogleFontsPreconnect/adds_only_one_preconnect/expected_output.html @@ -2,7 +2,7 @@ - +