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

Add amp-script minification #281

Merged
merged 11 commits into from
Aug 25, 2021
Merged

Conversation

ediamin
Copy link
Collaborator

@ediamin ediamin commented Jul 15, 2021

This PR adds the remaining script minification logic for the MinifyHtml transformer. The minification is optional and requires mck89/peast package. To enable the minification first install the mck89/peast package in your project and set minifyAmpScript = true of the MinifyHtmlConfiguration. For example,

$document = Document::fromHtml($source);

// Set the minifyAmpScript option
$configuration = new MinifyHtmlConfiguration([MinifyHtmlConfiguration::MINIFY_AMP_SCRIPT => true]);

$transformer = new MinifyHtml($configuration);
$errors  = new ErrorCollection();
$transformer->transform($document, $errors);
$document->saveHTML();

Related to #251

Fixes #260

Notes on ignoring the minifies_inline_amp-script spec test

Even we're using Peast to minify the amp script, the spect test is still ignored in this PR. The spec tests we are using in this repo are originated in the amp-toolbox repository and we are using the Terser package in that repo for the minification. Unfortunately, Peast and Terser work in very different ways and so we cannot use the test cases from the amp-toolbox repo in amp-toolbox-php. The output from the spec test by the terser is not the same when using the Peast. After some adjustment I was able to match the output with Peast except for a semicolon. The semicolon after 'Hello World!' in the following snippet from the spect test input.html cannot remove by the Peast.

<script id="hello-world" type="text/plain" target="amp-script">
  const btn = document.querySelector('button');
  btn.addEventListener('click', () => {
    document.body.textContent = 'Hello World!';
  });
</script>

The main problem is for the expression statement like document.body.textContent = 'Hello World!'; , there is no effect on the last semicolon. It doesn’t matter if we use the semicolon in input or not, Peast will always insert a semicolon at the end in output. The Renderer class decides whether the type of the node(in this case ExpressionStatement) requires semicolon insertion or not. This Renderer class has a fixed list of node types that do not need the semicolon. Also, the parser returns a Program node type at first and there is no override option to ignore adding semicolon for the whole statement either.

Then I tried to override the Renderer class, but that was not successful either. I cannot add the ExpressionStatement syntax class in noSemicolon list. It’ll remove the semicolon after 'Hello World' but it also removes any semicolon where it necessary after an ExpressionStatement.
I’ve tried to create a similar mechanism that we can find in terser, but it failed too. Because I cannot access the before or after character in Peast to check if I need to insert a semicolon.

Another thing I noticed is that, terser by default removes any dead or unused code, but Peast doesn’t. So, considering these situations, the spec test for the amp-script minification is still ignored.

@ediamin ediamin requested a review from schlessera July 15, 2021 09:49
Copy link
Collaborator

@schlessera schlessera left a comment

Choose a reason for hiding this comment

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

Some changes needed, mostly minor tweaks.

Also, the large piece of cruft at the end of the PR because of codecov is pretty annoying during a review. Please create a separate PR to disable the inline annotations of codecov in PRs.

composer.json Outdated Show resolved Hide resolved
src/Optimizer/Error/MissingPackage.php Outdated Show resolved Hide resolved
src/Optimizer/Transformer/MinifyHtml.php Outdated Show resolved Hide resolved
src/Optimizer/Transformer/MinifyHtml.php Outdated Show resolved Hide resolved
src/Optimizer/Transformer/MinifyHtml.php Outdated Show resolved Hide resolved
src/Optimizer/Transformer/MinifyHtml.php Outdated Show resolved Hide resolved
tests/Optimizer/Transformer/MinifyHtmlTest.php Outdated Show resolved Hide resolved
@ediamin ediamin force-pushed the add/260-peast-for-js-minification branch from 99859ff to 0e023d9 Compare July 30, 2021 07:01
@ediamin ediamin marked this pull request as ready for review August 10, 2021 11:58
@ediamin
Copy link
Collaborator Author

ediamin commented Aug 10, 2021

@schlessera I've updated the code and it is ready for your review. Also, I've added a note on why the spect test is still ignored in the main PR description.

src/Optimizer/Error/CannotMinifyAmpScript.php Outdated Show resolved Hide resolved
src/Optimizer/Error/CannotMinifyAmpScript.php Outdated Show resolved Hide resolved
src/Optimizer/Transformer/MinifyHtml.php Show resolved Hide resolved
tests/Optimizer/Transformer/MinifyHtmlTest.php Outdated Show resolved Hide resolved
@schlessera schlessera added this to the 0.7.0 milestone Aug 25, 2021
@schlessera schlessera merged commit 084b666 into main Aug 25, 2021
@schlessera schlessera deleted the add/260-peast-for-js-minification branch August 25, 2021 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing JavaScript minifier
2 participants