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.

Sign up for GitHub

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

fmigrate to htmlparser2 #576

Merged
merged 8 commits into from
Jan 22, 2020
Merged

fmigrate to htmlparser2 #576

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d60b748
Migrate to htmlparser2
sebastianbenz Jan 20, 2020
c95a9c7
Fix not all tags being copied to body
sebastianbenz Jan 21, 2020
69d20c7
addressed comments
sebastianbenz Jan 21, 2020
d00acb4
Remove nonces. See https://github.com/ampproject/amphtml/issues/26418
sebastianbenz Jan 21, 2020
0aeaee2
remove debug log
sebastianbenz Jan 21, 2020
1e59791
remove nonces from scripts in body
sebastianbenz Jan 21, 2020
1ee87f3
Search all of head for extensions to cope for broken heads
sebastianbenz Jan 21, 2020
25a9515
cleanup
sebastianbenz Jan 21, 2020
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
2 changes: 1 addition & 1 deletion jest.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ module.exports = {

// The glob patterns Jest uses to detect test files
testMatch: [
'**/?(*)+(Spec|spec).[tj]s?(x)',
'**/?(*)+(Spec|spec|test).[tj]s?(x)',
],

// An array of regexp pattern strings that are matched against all test paths, matched tests are skipped
Expand Down
57 changes: 54 additions & 3 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 5 additions & 8 deletions packages/cli/spec/cliSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,10 @@
const Cli = require('../lib/cli');
const MockLogger = require('./helpers/MockLogger');

describe('Cli', () => {
const mockLogger = new MockLogger();
const cli = new Cli(mockLogger);
const mockLogger = new MockLogger();
const cli = new Cli(mockLogger);

it('Triggers the correct command', (done) => {
return cli.run([])
.then(() => done())
.catch((e) => done.fail(e));
});
test('Triggers the correct command', async () => {
await cli.run([]);
// no exception thrown
});
2 changes: 1 addition & 1 deletion packages/cli/spec/cmds/optimizeSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ describe('optimize', () => {

it('runs optimizer', () => {
expect(
mockLogger.getLogs().startsWith('<!DOCTYPE html><html amp="" i-amphtml-layout'),
mockLogger.getLogs().startsWith('<!doctype html><html amp i-amphtml-layout'),
).toBe(true);
});
it('loads url / path', () => {
Expand Down
40 changes: 18 additions & 22 deletions packages/cli/spec/ioSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,26 +18,22 @@

const {loadUrlOrFile} = require('../lib/io');

describe('io', () => {
describe('loadUrlOrFile', async () => {
it('loads url', async () => {
expect(
await loadUrlOrFile('https://amp.dev/documentation/examples/api/echo?hello=world'),
).toBe('{"hello":"world"}');
});
it('loads file', async () => {
expect(
await loadUrlOrFile(__dirname + '/test-data/hello.txt'),
).toBe('hello\n');
});
it('fails if url is missing', async () => {
let error;
try {
await loadUrlOrFile('');
} catch (e) {
error = e;
}
expect(error).toEqual(new Error('Missing URL or file path'));
});
});
test('loads url', async () => {
expect(
await loadUrlOrFile('https://amp.dev/documentation/examples/api/echo?hello=world'),
).toBe('{"hello":"world"}');
});
test('loads file', async () => {
expect(
await loadUrlOrFile(__dirname + '/test-data/hello.txt'),
).toBe('hello\n');
});
test('fails if url is missing', async () => {
let error;
try {
await loadUrlOrFile('');
} catch (e) {
error = e;
}
expect(error).toEqual(new Error('Missing URL or file path'));
});
4 changes: 2 additions & 2 deletions packages/optimizer/demo/cheerio/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,11 @@ class CheerioTransformer {
* DomTransformer.transformHtml(), and is (probably) an object of key-value
* pairs.
*
* @param {parse5.AST.HtmlParser2.Document} tree a DOM tree
* @param {NodeWithChildren} tree a DOM tree
* @param {Object} params transformer options
*/
transform(tree, params) {
const $ = cheerio.load(tree.root.children);
const $ = cheerio.load(tree.children);
// Prepends "Optimized" to the <title>
$('title').text('Optimized: ' + $('title').text());
// Injects amp-fx-parallax component
Expand Down
9 changes: 5 additions & 4 deletions packages/optimizer/demo/simple/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const path = require('path');
const mkdirp = require('mkdirp');

const AmpOptimizer = require('../../index.js');
const {createElement, firstChildByTag, appendChild} = require('../../lib/NodeUtils.js');

const runtimeVersion = require('@ampproject/toolbox-runtime-version');

Expand Down Expand Up @@ -64,15 +65,15 @@ async function customAmpTransformation(filePath, html) {
}
transform(tree, params) {
this.log_.info('Running custom transformation for ', params.filePath);
const html = tree.root.firstChildByTag('html');
const html = firstChildByTag(tree, 'html');
Copy link
Member

Choose a reason for hiding this comment

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

nit: I do prefer firstChildByTag(tree, 'html') instead of tree.root.firstChildByTag('html'). I wonder if we could have tree.firstChildByTab('html'), similar to the call on line 69.

if (!html) return;
const head = html.firstChildByTag('head');
const head = firstChildByTag(html, 'head');
if (!head) return;
const desc = tree.createElement('meta', {
const desc = createElement('meta', {
name: 'description',
content: 'this is just a demo',
});
head.appendChild(desc);
appendChild(head, desc);
}
}

Expand Down
17 changes: 12 additions & 5 deletions packages/optimizer/lib/DomTransformer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@
*/
'use strict';

const treeParser = require('./TreeParser.js');
const log = require('./log.js');
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');
const RuntimeVersion = require('@ampproject/toolbox-runtime-version/lib/RuntimeVersion');

/**
* AMP Optimizer Configuration only applying AMP validity perserving transformations.
Expand All @@ -41,9 +41,13 @@ const TRANSFORMATIONS_AMP_FIRST = [
'RewriteAmpUrls',
'GoogleFontsPreconnect',
'PruneDuplicateResourceHints',
// Move keyframes into a separate style tag
'SeparateKeyframes',
'AddTransformedFlag',
// Inject CSP script has required for inline amp-script
'AmpScriptCsp',
// Removes unsupported nonce attribute from scripts
'RemoveCspNonce',
];

/**
Expand Down Expand Up @@ -79,7 +83,6 @@ const TRANSFORMATIONS_PAIRED_AMP = [
const DEFAULT_CONFIG = {
fetch: oneBehindFetch,
log,
runtimeVersion,
transformations: TRANSFORMATIONS_AMP_FIRST,
validatorRules,
verbose: false,
Expand All @@ -105,7 +108,7 @@ class DomTransformer {
* @return {string} - the transformed html string
*/
async transformHtml(html, params) {
const tree = treeParser.parse(html);
const tree = await treeParser.parse(html);
await this.transformTree(tree, params);
return treeParser.serialize(tree);
}
Expand Down Expand Up @@ -133,6 +136,10 @@ class DomTransformer {
*/
setConfig(config) {
config = Object.assign({}, DEFAULT_CONFIG, config);
if (!config.runtimeVersion) {
// Re-use custom fetch implementation for runtime version provider
config.runtimeVersion = new RuntimeVersion(config.fetch);
}
log.verbose(config.verbose);
this.initTransformers_(config);
}
Expand Down
Loading