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

Omit auto-generated tags #540

Merged
merged 3 commits into from
Mar 10, 2016
Merged

Omit auto-generated tags #540

merged 3 commits into from
Mar 10, 2016

Conversation

alexlamsl
Copy link
Collaborator

Implemented an option to suppress synthetic tags generated by htmlparser from showing up in the minified output.

While it is useful for the parser to generate these signals for minifier to work within the correct context, they do have the effects of breaking browser hacks and other intentional markup omissions.

As it is guarded behind ignoreSyntheticTags and is turned off by default, we should be backward-compatible.

Also take out cleanAttributes from tests since the option no longer exists.

@alexlamsl alexlamsl mentioned this pull request Mar 8, 2016
@@ -1962,6 +1962,22 @@
equal(minify(input, options), output);
});

test('ignore synthetic tags', function() {
equal(minify('<p id=""class=""title="">x', { ignoreSyntheticTags: true }), '<p id="" class="" title="">x');
Copy link
Owner

Choose a reason for hiding this comment

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

How would the output be different with ignoreSyntheticTags: false?

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'd add </p> - I'll add a further test for clarification.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks. Yeah, it's not really intuitive to me what syntheticTags mean. Could we perhaps say autoGenerated or autoClosing? Or is "synthetic" the actual term from the spec?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I made that term up, and I'm notoriously bad at naming things 😅

autoGenerated sounds generic enough for possible further expansion - though in this case the default would be true, i.e. it'd be the opposite sense to the current setting. Is that okay?

Copy link
Owner

Choose a reason for hiding this comment

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

how about includeAutoGeneratedTags that's true by default?

@alexlamsl
Copy link
Collaborator Author

@kangax flag (and commit message) renamed, tests added

kangax added a commit that referenced this pull request Mar 10, 2016
@kangax kangax merged commit 6f89b55 into kangax:gh-pages Mar 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants