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

fmigrate to htmlparser2 #576

Merged
merged 8 commits into from
Jan 22, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 1 addition & 1 deletion jest.config.js
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

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
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
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
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'));
});
2 changes: 1 addition & 1 deletion packages/optimizer/demo/simple/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ 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');
Copy link
Member

Choose a reason for hiding this comment

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

I think this is the only place firstChildByTag is being used in this format. Wondering if this is actually wrong?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup, wrong

if (!head) return;
Expand Down
6 changes: 3 additions & 3 deletions packages/optimizer/lib/DomTransformer.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
*/
'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');
Expand Down Expand Up @@ -105,7 +105,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
199 changes: 199 additions & 0 deletions packages/optimizer/lib/NodeUtils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,199 @@
/**
* 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.
*/
'use strict';

const {Element, DataNode} = require('domhandler');
const domUtils = require('domutils');
Copy link
Member

Choose a reason for hiding this comment

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

We're requiring the entire domutils here and then a few selected methods below. Do we need both?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's to avoid a name conflict

const {removeElement, append, prepend} = require('domutils');

/**
* Depth-first walk through the DOM tree.
* @param {Node} node
*/
const nextNode = function(node) {
// Walk downwards if there are children
const firstChild = node.firstChild;
if (firstChild) {
return firstChild;
}
// Return the direct sibling or walk upwards until we find a node with sibling
let tmp = node;
while (tmp) {
const next = tmp.nextSibling;
if (next) {
return next;
}
// Walk upwards
tmp = tmp.parent;
}
// We are done
return null;
};


/**
* Remove node from DOM
*
* @param {Node} node
*/
const remove = function(node) {
removeElement(node);
};

/**
* Appends a node to the given parent
*
* @param {Node} parent
* @param {Node} node
*/
const appendChild = function(parent, node) {
if (!node) {
return;
}
domUtils.appendChild(parent, node);
};

/**
* Inserts a Node before the reference node. If referenceNode is null, inserts the node as
* the first child.
*
* @param {Node} newNode the node to be inserted.
* @param {Node} referenceNode the reference node, where the new node will be added before.
*/
const insertBefore = function(parent, newNode, referenceNode) {
if (referenceNode) {
prepend(referenceNode, newNode);
return;
}
// if referenceNode.nextSibling is null, referenceNode is the last child. newNode is inserted
// as the last element.
appendChild(parent, newNode);
};

/**
* Inserts a Node after the reference node. If referenceNode is null, inserts the node as
* the first child.
*
* @param {Node} newNode the node to be inserted.
* @param {Node} referenceNode the reference node, where the new node will be added after.
*/
const insertAfter = function(parent, newNode, referenceNode) {
if (referenceNode) {
append(referenceNode, newNode);
return;
}
// if referenceNode.nextSibling is null, referenceNode is the last child. newNode is inserted
// as the last element.
appendChild(parent, newNode);
};

/**
* Appends all nodes to the given parent
*
* @param {Node} parent
* @param {Array<Node>} node
*/
const appendAll = function(node, nodes) {
if (!nodes) {
return;
}
for (let i = 0, len = nodes.length; i < len; i++) {
appendChild(node, nodes[i]);
}
};

/**
* Returns the first child with the given tag name
*
* @param {Node} node
* @param {String} tagName
*/
const firstChildByTag = function(node, tagName) {
if (!node.children) {
return null;
}
return node.children.find(
(child) => child.tagName && child.tagName === tagName,
);
};

//
/**
* Returns true if an attribute with the given name exists
* @param {Node} node
* @param {String} attributeName
*/
const hasAttribute = function(node, attribute) {
if (!node.attribs) return false;
return attribute in node.attribs;
};

/**
* Move a node from one parent to another
* @param {Node} nodeToMove
* @param {Node} newParent
*/
const move = function(nodeToMove, newParent) {
remove(nodeToMove);
appendChild(newParent, nodeToMove);
};

/**
* Creates a new element
*
* @param {string} tagName
* @param {obj} [attribs={}]
* @returns {Node} new node
*/
const createElement = (tagName, attribs) => {
return new Element(tagName, attribs);
};

/**
* Inserts text
*
* @param {Node} node
* @param {string} the text
*/
const insertText = (node, text) => {
const dataNode = new DataNode('text', text);
appendChild(node, dataNode);
};

/**
* Creates a new doctype
*/
const createDocType = () => {
const result = new DataNode('directive', '!doctype html');
return result;
};


module.exports = {
appendChild,
appendAll,
insertAfter,
nextNode,
remove,
createDocType,
createElement,
insertText,
insertBefore,
hasAttribute,
firstChildByTag,
move,
}
;
Loading