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

Ignore Apache SSI directives by default #39

Merged
merged 1 commit into from Apr 29, 2020
Merged

Ignore Apache SSI directives by default #39

merged 1 commit into from Apr 29, 2020

Conversation

textbook
Copy link
Contributor

@textbook textbook commented Apr 29, 2020

It seems likely that anyone including these directives wants to retain them in the output, and changing the default rather than requiring explicit configuration supports folks using e.g. Create React App.

Also I should note that the extensive test coverage and well-configured automation made this codebase an absolute pleasure to work with, so thanks for your efforts on that front!

@@ -26083,7 +26083,10 @@ function processOptions(values) {
canCollapseWhitespace: canCollapseWhitespace,
canTrimWhitespace: canTrimWhitespace,
html5: true,
ignoreCustomComments: [/^!/],
ignoreCustomComments: [
Copy link
Collaborator

@DanielRuf DanielRuf Apr 29, 2020

Choose a reason for hiding this comment

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

Please revert the changes on the dist files. These are built on release.

@DanielRuf
Copy link
Collaborator

DanielRuf commented Apr 29, 2020

Hi @textbook,

thanks for your PR.
Please revert the dist changes.

I guess this is a SemVer-minor update then as this should not break any setups.
But I'm not sure as we change defaults which is an API change and so it is a breaking change because the default output changes because of this, which would require a SemVer-major release.

It seems likely that anyone including these directives wants to retain them in
the output, and changing the default rather than requiring explicit
configuration support folks using e.g. Create React App.
@textbook
Copy link
Contributor Author

textbook commented Apr 29, 2020

Updated; sorry, as the tests built the dist/ and it wasn't .gitignored I assume I should include it.


In terms of semver, that's a tricky one. I wouldn't necessarily consider changing a default an API change (unlike e.g. renaming or removing the configuration option). Let's think about the groups of users:

Config \ Directives Yes No
Explicit No change No change
Default Output changes No change

My guess would be that the group that has directives and is using the default configuration will be pretty small, maybe even none. Only people who were relying on this tool to strip out those directives but didn't explicitly set ignoreCustomComments to [] or false would see any changes to their output, and that change only alters any behaviour if, despite wanting SSI to definitely not happen, it was enabled on the server.

My vote would be for minor, on that basis, but I'd be open to hearing opinions from others.

@DanielRuf
Copy link
Collaborator

DanielRuf commented Apr 29, 2020

My guess would be that the group that has directives and is using the default configuration will be pretty small, maybe even none. Only people who were relying on this tool to strip out those directives but didn't explicitly set ignoreCustomComments to [] or false would see any changes to their output, and that change only alters any behaviour if, despite wanting SSI to definitely not happen, it was enabled on the server.

Only if no other comments with # are there ;-)
I thin a minor release should be fine then.

Copy link
Collaborator

@DanielRuf DanielRuf left a comment

Thanks for your contribution.

@DanielRuf DanielRuf added this to In progress in development via automation Apr 29, 2020
@DanielRuf DanielRuf self-assigned this Apr 29, 2020
@DanielRuf DanielRuf merged commit d5ec126 into terser:master Apr 29, 2020
development automation moved this from In progress to Done Apr 29, 2020
@DanielRuf
Copy link
Collaborator

DanielRuf commented Apr 29, 2020

The changes were released with html-minifier-terser v5.1.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
development
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants