Add importmap minification #172
Conversation
README.md
Outdated
| @@ -97,7 +97,7 @@ Most of the options are disabled by default. | |||
| | `preserveLineBreaks` | Always collapse to 1 line break (never remove it entirely) when whitespace between tags include a line break. Must be used in conjunction with `collapseWhitespace=true` | `false` | | |||
| | `preventAttributesEscaping` | Prevents the escaping of the values of attributes | `false` | | |||
| | `processConditionalComments` | Process contents of conditional comments through minifier | `false` | | |||
| | `processScripts` | Array of strings corresponding to types of script elements to process through minifier (e.g. `text/ng-template`, `text/x-handlebars-template`, etc.) | `[ ]` | | |||
| | `processScripts` | Array of strings corresponding to types of script elements to process through minifier (e.g. `text/ng-template`, `text/x-handlebars-template`, `importmap`, etc.) | `[ ]` | | |||
There was a problem hiding this comment.
Personally I think we don't have to mention the type there.
README.md
Outdated
| @@ -97,7 +97,7 @@ Most of the options are disabled by default. | |||
| | `preserveLineBreaks` | Always collapse to 1 line break (never remove it entirely) when whitespace between tags include a line break. Must be used in conjunction with `collapseWhitespace=true` | `false` | | |||
| | `preventAttributesEscaping` | Prevents the escaping of the values of attributes | `false` | | |||
| | `processConditionalComments` | Process contents of conditional comments through minifier | `false` | | |||
| | `processScripts` | Array of strings corresponding to types of script elements to process through minifier (e.g. `text/ng-template`, `text/x-handlebars-template`, etc.) | `[ ]` | | |||
| | `processScripts` | Array of strings corresponding to types of script elements to process through minifier (e.g. `text/ng-template`, `text/x-handlebars-template`, `importmap`, etc.) | `[ ]` | | |||
There was a problem hiding this comment.
Personally I think we don't have to mention the type there.
| function minifyImportmap(value, options) { | ||
| if (options.collapseWhitespace) { | ||
| try { | ||
| return JSON.stringify(JSON.parse(value)); |
There was a problem hiding this comment.
I'm surprised that we need this approach even if we already can minify JSON-LD / microdata.
See also:
#146
#34
https://user-images.githubusercontent.com/827205/224982981-01f104bd-4033-4ac5-a22c-706c48c4199c.png
The difference to the available option is not quite clear to me:
Can you clarify the difference and why we need this extra function here?
There was a problem hiding this comment.
$ printf '
<script type="importmap">
{
"imports": {
"shapes/": "./module/shapes/",
"othershapes/": "https://example.com/modules/shapes/"
}
}
</script>
' | npx html-minifier-terser --process-scripts 'importmap' --collapse-whitespace<script type="importmap">{ "imports": { "shapes/": "./module/shapes/", "othershapes/": "https://example.com/modules/shapes/" } }</script>with the patch you will get:
<script type="importmap">{"imports":{"shapes/":"./module/shapes/","othershapes/":"https://example.com/modules/shapes/"}}</script>Notice the minified JSON.
$ printf '
<script type="application/ld+json">
{
"@context": "https://json-ld.org/contexts/person.jsonld",
"@id": "http://dbpedia.org/resource/John_Lennon",
"name": "John Lennon",
"born": "1940-10-09",
"spouse": "http://dbpedia.org/resource/Cynthia_Lennon"
}
</script>
' | npx html-minifier-terser --process-scripts 'application/ld+json' --collapse-whitespace<script type="application/ld+json">{ "@context": "https://json-ld.org/contexts/person.jsonld", "@id": "http://dbpedia.org/resource/John_Lennon", "name": "John Lennon", "born": "1940-10-09", "spouse": "http://dbpedia.org/resource/Cynthia_Lennon" }</script>I could adjust the patch to also route application/ld+json (or everything with +json at the end) into minifyImportmap -- in that case I would rename it to minifyJSON though.
Another alternative would be to introduce a new CLI argument --process-json-scripts (or --process-scripts-json) which would handle both importmap and application/ld+json for now.
The implementation would have an array with importmap and application/ld+json and we would add other micro formats on demand to that array.
Or the CLI parameter could take a argument and behave similar to --process-scripts in that you explicitly have to state them.
If we add a new CLI parameter we have to decide what happens when --process-scripts also contains the script type, in my opinion it should ignore it.
There was a problem hiding this comment.
if we also route application/ld+json into minifyImportmap aka minifyJSON we would solve #146 (comment)
There was a problem hiding this comment.
I don't think this is the right approach. We should try to find a better way to handle the spaces (which seem to be the only difference). Besides that, html-minifier-terser already supports importmap minification as outlined in my other comments.
There might be extra settings that you need to remove the extra spaces or we have to fix some logic. For me the extra function makes not much sense, because there are more cases and JSON-LD was just one example.
The parser should treat similar inputs the same way, independent of the value of the type attribute. A solution via JSON.stringify does not feel right, because it does not solve the underlying root cause for the extra spaces.
There was a problem hiding this comment.
I added some more tests, I will look at a different approach for processScript over the weekend. The documentation has to be updated after ...
There was a problem hiding this comment.
I have improved the tests and updated the documentation.
JSON.stringify in combination with JSON.parse does more than just remove spaces:
$ node -e "console.log(JSON.stringify(JSON.parse('{ \"key\": \"ignored\", \"key\": -2.1E+35}')))"
{"key":-2.1e+35}{ "key": "ignored", "key": -2.1E+35}
{ "key": -2.1e+35}
⇒ the duplicate key has been removed and the E has been lower-cased
There was a problem hiding this comment.
We could have one option just to remove the whitespaces and another to use JSON.stringify.
Signed-off-by: Sebastian Davids <sdavids@gmx.de>
|
I very much appreciate your effort and the amount of work that you have invested. Still, I think this is not going into the right direction and leads to more edge cases and all this just to get rid of extra spaces. We just need an option to clear these extra spaces (in general), for all inputs (nut just JSON or importmap). The JSON.parse/stringify workaround is not an ideal solution. Regarding tests, I think real examples make more sense. |
fixes #169