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 MinifyHtml to list of default transformers #330

Merged
merged 1 commit into from
Aug 25, 2021

Conversation

schlessera
Copy link
Collaborator

This enables HTML minification by default.

Currently, the amp-script minification logic is disabled by default, as it requires an external dependency: mck89/peast.

@schlessera
Copy link
Collaborator Author

@westonruter The minification of JSON is currently enabled by default. I've read through https://wordpress.org/support/topic/alexa-atrk_acct-id-slash-encoding-adding-backslash/, and as far as I can tell, there's nothing stating that the spacing was the issue, but rather the extra backslash. Therefore, I think we should be relatively safe to enable JSON modification for now. Thoughts?

Transformer\RewriteAmpUrls::class,
Transformer\ReorderHead::class,
Transformer\OptimizeAmpBind::class,
Transformer\MinifyHtml::class,
Copy link
Collaborator Author

@schlessera schlessera Aug 25, 2021

Choose a reason for hiding this comment

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

Not super obvious from the PR diff, but Transformer\MinifyHtml::class is an addition here, whereas the rest was just adapted to make use of relative namespaces.

@westonruter
Copy link
Member

@westonruter The minification of JSON is currently enabled by default. I've read through https://wordpress.org/support/topic/alexa-atrk_acct-id-slash-encoding-adding-backslash/, and as far as I can tell, there's nothing stating that the spacing was the issue, but rather the extra backslash. Therefore, I think we should be relatively safe to enable JSON modification for now. Thoughts?

Does it but add slashes to JSON when reserializing after parsing?

@schlessera
Copy link
Collaborator Author

Apparently, it does: https://3v4l.org/ZdYA1#v8.0.9 😞

Image 2021-08-25 at 10 39 13 PM

@schlessera
Copy link
Collaborator Author

But we could actually remove that behavior via JSON_UNESCAPED_SLASHES:

Image 2021-08-25 at 10 40 40 PM

@schlessera
Copy link
Collaborator Author

Ah, we're already using that, so we should be good:

Image 2021-08-25 at 10 42 14 PM

@westonruter
Copy link
Member

OK cool, and there's no risk that "</script>" will get serialized to break out of a script tag due to JSON_HEX_TAG, which was my concern about unescaped slashes. So this seems fine.

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.

2 participants