Skip to content

Conversation

@aminimalanimal
Copy link
Contributor

@aminimalanimal aminimalanimal commented Jan 13, 2020

I am using this package to minify code that will be copy/pasted into a custom template in a CMS. This CMS has stipulations for what it considers valid, and I therefore needed to turn off certain options so that the resulting file passes the CMS's tests. In particular, it was necessary to keep attribute quotes and keep the closing body and html tags intact.

This PR allows options to be passed in via the JavaScript API. The user's options are then combined with the default options before being passed to the HTML minifier.

This also resolves #37, which was already closed, but was looking for the same sort of feature. It might resolve #24 as well.

@coderaiser
Copy link
Owner

This is a very good idea, but would be great to have ability to configure all minifiers, not only html :).
The simplest way would be to split options into 4 categories:

  • js
  • css
  • html
  • img

So example would look like this:

const html = {
    removeAttributeQuotes: false,
    removeOptionalTags: false
};

const options = {
    html,
};

const result = await minify('./client.js', options)
console.log(result);

What do you think about such API :)?

@aminimalanimal
Copy link
Contributor Author

Sounds reasonable enough. I think I'll have time to do this tomorrow.

@aminimalanimal
Copy link
Contributor Author

Updates made. I was able to locally validate that HTML, CSS, and JS options worked. My particular use-case doesn't have any images its passing through, but it works the same, so it should work.

@coderaiser
Copy link
Owner

That's amazing :), could you please add a couple tests for this?

@aminimalanimal
Copy link
Contributor Author

I considered that, but hesitated. Given that minify's only job in this regard is to pass the options to other tools, and that the APIs are defined entirely by the other tools, it makes sense that minify would not want to fail any particular test due to one of its dependencies changing their API. So the only thing minify would want to test is whether another one of the tools successfully received the options, right? And I'm not exactly sure how to test that.

Maybe I'm overthinking it. Could use some brainstorming.

@coderaiser
Copy link
Owner

The simplest way to test things, would be to pass options into minify and call real API with the same options, and check that output is the same :). One test for each html, js, css and if it's not hard for img into existing test/minify.js would be enough :).

…correct tests that were double-minifying items; update naming conventions in tests
@aminimalanimal
Copy link
Contributor Author

I've added unit tests following this direction, and also added a notEqual check (comparing versions with options to versions without options) to ensure the options are causing changes. I didn't add a test for img.

@aminimalanimal
Copy link
Contributor Author

I also noticed that some of the existing tests were passing minify's output into terser/css-clean (effectively re-minifying items) instead of testing the output of the original input into both, so I fixed that. Hopefully I've correctly understood the intention.

@coderaiser
Copy link
Owner

That's amazing, thank you :)!

@coderaiser coderaiser merged commit 2c10057 into coderaiser:master Jan 22, 2020
@coderaiser
Copy link
Owner

Landed in v5.1.0 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HTML minification strips head tag and closing /body and /html tags turn off uglify?

2 participants