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

Extract runtime parameters #736

Merged
merged 5 commits into from
May 4, 2020
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/optimizer-express/demo/proxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ const apicache = require('apicache');
const cache = apicache.middleware;

// Read proxy for command line, or default to ampbyexample.com.
const target = process.argv[2] || 'https://www.ampbyexample.com';
const target = process.argv[2] || 'https://amp.dev';

// Create a HTTP Proxy server the target
const proxy = httpProxy.createProxyServer({
Expand Down
1 change: 1 addition & 0 deletions packages/optimizer/lib/AmpConstants.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,6 @@ module.exports = {
AMP_TAGS: ['amp', '⚡', '⚡4ads', 'amp4ads', '⚡4email', 'amp4email'],
AMP_CACHE_HOST: 'https://cdn.ampproject.org',
AMP_FORMATS: ['AMP', 'AMP4EMAIL', 'AMP4ADS'],
AMP_RUNTIME_CSS_PATH: '/v0.css',
appendRuntimeVersion: (prefix, version) => prefix + '/rtv/' + version,
};
24 changes: 12 additions & 12 deletions packages/optimizer/lib/DomTransformer.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@
const treeParser = require('./TreeParser');
const log = require('./log');
const {oneBehindFetch} = require('@ampproject/toolbox-core');
const validatorRules = require('@ampproject/toolbox-validator-rules');
const RuntimeVersion = require('@ampproject/toolbox-runtime-version/lib/RuntimeVersion');
const fetchRuntimeParameters = require('./fetchRuntimeParameters');

/**
* AMP Optimizer Configuration only applying AMP validity perserving transformations.
Expand Down Expand Up @@ -94,7 +94,6 @@ const DEFAULT_CONFIG = {
fetch: oneBehindFetch,
log,
transformations: TRANSFORMATIONS_AMP_FIRST,
validatorRules,
verbose: false,
};

Expand Down Expand Up @@ -126,14 +125,14 @@ class DomTransformer {
/**
* Transforms a DOM tree.
* @param {Tree} tree - a DOM tree.
* @param {Object} params - a dictionary containing transformer specific parameters.
* @param {Object} customParams - a dictionary containing transformer specific parameters.
*/
transformTree(tree, params) {
params = params || {};
log.verbose(params.verbose || false);
async transformTree(tree, customParams = {}) {
const runtimeParameters = await fetchRuntimeParameters(this.config, customParams);
log.verbose(runtimeParameters.verbose);
const sequence = async (promise, transformer) => {
await promise;
return transformer.transform(tree, params);
return transformer.transform(tree, runtimeParameters);
};
return this.transformers_.reduce(sequence, Promise.resolve());
}
Expand All @@ -142,16 +141,17 @@ class DomTransformer {
* Set the config.
* @param {Object} config - The config.
* @param {boolean} config.verbose - true if verbose mode should be enabled [default: false].
* @param {Object} config.fetch - the fetch implementation to use.
* @param {Array.<Transformer>} config.transformations - a list of transformers to be applied.
*/
setConfig(config) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I might opt for this.config = this.setConfig(config); in the constructor so that it's easier to identify that DomTransformer has property config available without digging into a function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This duplicates initializing the field in the constructor and in the setter. Imo setX implies that there's a field (my Java roots might be showing trough here...)

config = Object.assign({}, DEFAULT_CONFIG, config);
if (!config.runtimeVersion) {
this.config = Object.assign({}, DEFAULT_CONFIG, config);
if (!this.config.runtimeVersion) {
// Re-use custom fetch implementation for runtime version provider
config.runtimeVersion = new RuntimeVersion(config.fetch);
this.config.runtimeVersion = new RuntimeVersion(this.config.fetch);
}
log.verbose(config.verbose);
this.initTransformers_(config);
log.verbose(this.config.verbose);
this.initTransformers_(this.config);
}

/**
Expand Down
4 changes: 2 additions & 2 deletions packages/optimizer/lib/RuntimeHostHelper.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@

const {AMP_CACHE_HOST, appendRuntimeVersion} = require('./AmpConstants.js');

function calculateHost({ampUrlPrefix = AMP_CACHE_HOST, ampRuntimeVersion, lts}) {
function calculateHost({ampUrlPrefix = AMP_CACHE_HOST, ampRuntimeVersion, lts, rtv = false}) {
if (lts && ampRuntimeVersion) {
throw new Error('lts flag is not compatible with runtime version parameter');
}

ampUrlPrefix = ampUrlPrefix.replace(/\/$/, '');
if (ampRuntimeVersion) {
if (ampRuntimeVersion && rtv) {
ampUrlPrefix = appendRuntimeVersion(ampUrlPrefix, ampRuntimeVersion);
} else if (lts) {
ampUrlPrefix += '/lts';
Expand Down
96 changes: 96 additions & 0 deletions packages/optimizer/lib/fetchRuntimeParameters.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
/**
* Copyright 2020 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.
*/
'mode strict';

const validatorRules = require('@ampproject/toolbox-validator-rules');
const {AMP_CACHE_HOST, AMP_RUNTIME_CSS_PATH, appendRuntimeVersion} = require('./AmpConstants.js');

/**
* Initializes the runtime parameters used by the transformers based on given config and parameter values.
* If missing, the following parameters are fetched from cdn.ampproject.org:
*
* - validatorRules: the latest version of the AMP validator rules as served from https://cdn.ampproject.org/v0/validator.json
* - ampRuntimeVersion: the latest AMP runtime version or the latest lts version if the lts flag is set
* - ampRuntimeStules: the latest AMP runtime CSS styles or the latest lts version if the lts flag is set
*
* @param {Object} config - the AMP Optimizer config
* @param {Object} customRuntimeParameters - user defined runtime parameters
* @returns {Promise<Object>} - the runtime parameters
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

You might consider adding a @returns to quickly identify the parameters that are typically returned. It's a bit difficult to identify them all by reading through the code. Similarly, in the description, you might mention which parameters are simply validated, which parameters are fetched and verified via network, and the rest pass-through untouched.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

async function fetchRuntimeParameters(config, customRuntimeParameters) {
const runtimeParameters = Object.assign({}, customRuntimeParameters);
// Configure the log level
runtimeParameters.verbose = customRuntimeParameters.verbose || config.verbose || false;
// Copy lts and rtv runtime flag from custom parameters or the static config. Both are disabled by default.
runtimeParameters.lts = customRuntimeParameters.lts || config.lts || false;
runtimeParameters.rtv = customRuntimeParameters.rtv || config.rtv || false;
// Fetch the validator rules
runtimeParameters.validatorRules = config.validatorRules || (await validatorRules.fetch());
let {ampUrlPrefix, ampRuntimeVersion, ampRuntimeStyles, lts} = runtimeParameters;
// Use existing runtime version or fetch lts or latest
runtimeParameters.ampRuntimeVersion =
ampRuntimeVersion || (await config.runtimeVersion.currentVersion({ampUrlPrefix, lts}));
// Fetch runtime styles based on the runtime version
runtimeParameters.ampRuntimeStyles =
ampRuntimeStyles ||
(await fetchAmpRuntimeStyles_(config, ampUrlPrefix, runtimeParameters.ampRuntimeVersion));
return runtimeParameters;
}

/**
* @private
*/
async function fetchAmpRuntimeStyles_(config, ampUrlPrefix, ampRuntimeVersion) {
if (ampUrlPrefix && !_isAbsoluteUrl(ampUrlPrefix)) {
config.log.warn(
`AMP runtime styles cannot be fetched from relative ampUrlPrefix, please use the 'ampRuntimeStyles' parameter to provide the correct runtime style.`
);
// Gracefully fallback to latest runtime version
ampUrlPrefix = AMP_CACHE_HOST;
ampRuntimeVersion = ampRuntimeVersion || (await config.runtimeVersion.currentVersion());
}
// Construct the AMP runtime CSS download URL, the default is: https://cdn.ampproject.org/rtv/${ampRuntimeVersion}/v0.css
const runtimeCssUrl =
appendRuntimeVersion(ampUrlPrefix || AMP_CACHE_HOST, ampRuntimeVersion) + AMP_RUNTIME_CSS_PATH;
// Fetch runtime styles
const response = await config.fetch(runtimeCssUrl);
if (!response.ok) {
config.log.error(
`Could not fetch ${runtimeCssUrl}, failed with status ` + `${response.status}.`
);
if (ampUrlPrefix || ampRuntimeVersion) {
// Try to download latest from cdn.ampproject.org instead
return fetchAmpRuntimeStyles_(AMP_CACHE_HOST, await config.runtimeVersion.currentVersion());
} else {
return '';
}
}
return response.text();
}

/**
* @private
*/
function _isAbsoluteUrl(url) {
try {
new URL(url);
return true;
} catch (ex) {}

return false;
}

module.exports = fetchRuntimeParameters;
76 changes: 14 additions & 62 deletions packages/optimizer/lib/transformers/AmpBoilerplateTransformer.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,7 @@ const {
hasAttribute,
firstChildByTag,
} = require('../NodeUtils');
const {AMP_CACHE_HOST, appendRuntimeVersion} = require('../AmpConstants.js');

const V0_CSS_PATH = '/v0.css';
const {AMP_CACHE_HOST, V0_CSS_PATH, appendRuntimeVersion} = require('../AmpConstants.js');

/**
* AmpBoilerplateTransformer - This DOM transformer adds
Expand All @@ -47,11 +45,21 @@ class AmpBoilerplateTransformer {
return; // invalid doc
}
// amp-runtime is added by server-side-rendering
const ampRuntimeStyle = this._findAmpRuntimeStyle(head);
if (!ampRuntimeStyle) {
const ampRuntimeStylesNode = this._findAmpRuntimeStyle(head);
if (!ampRuntimeStylesNode) {
return; // keep existing boilerplate
}
return this._addStaticCss(ampRuntimeStyle, params);
// inline CSS
let {ampRuntimeVersion, ampRuntimeStyles} = params;
if (!ampRuntimeVersion || !ampRuntimeStyles) {
// these should be set by DomTransformer
this.log_.error(
'Missing parameters both ampRuntimeVersion and ampRuntimeStyles need to be present'
);
return;
}
ampRuntimeStylesNode.attribs['i-amphtml-version'] = ampRuntimeVersion;
insertText(ampRuntimeStylesNode, ampRuntimeStyles);
}

_findAmpRuntimeStyle(head) {
Expand All @@ -64,62 +72,6 @@ class AmpBoilerplateTransformer {
}
return null;
}

async _addStaticCss(node, params) {
// we can always inline v0.css as the AMP runtime will take care of keeping v0.css in sync
try {
return this._inlineCss(node, params);
} catch (error) {
this.log_.error(error);
this._linkCss(node);
}
}

_linkCss(node) {
const cssStyleNode = createElement('link', {
rel: 'stylesheet',
href: AMP_CACHE_HOST + V0_CSS_PATH,
});
insertBefore(node.parent, cssStyleNode, node);
}

async _inlineCss(node, params) {
let {ampRuntimeVersion, ampUrlPrefix, lts} = params;

// TODO: If ampUrlPrefix is a relative URL, this will fall back to
// fetching the latest runtime version and boilerplate CSS from
// cdn.ampproject.org. Is this our best option?
if (ampUrlPrefix && !this._isAbsoluteUrl(ampUrlPrefix)) {
ampUrlPrefix = undefined;
ampRuntimeVersion = undefined;
}

const version =
ampRuntimeVersion || (await this.runtimeVersion_.currentVersion({ampUrlPrefix, lts}));
const v0CssUrl = appendRuntimeVersion(ampUrlPrefix || AMP_CACHE_HOST, version) + V0_CSS_PATH;

node.attribs['i-amphtml-version'] = version;

// Fetch and inline contents of v0.css
const response = await this.fetch_(v0CssUrl);
if (!response.ok) {
throw new Error(
`Could not inline v0.css. Request to ${v0CssUrl} failed with status ` +
`${response.status}.`
);
}
const body = await response.text();
insertText(node, body);
}

_isAbsoluteUrl(url) {
try {
new URL(url);
return true;
} catch (ex) {}

return false;
}
}

module.exports = AmpBoilerplateTransformer;