rehypejs / rehype-minify

plugins to minify HTML
https://unifiedjs.com
MIT License
89 stars 16 forks source link

rehype-minify-whitespace collapses inline javascript wrongly. #44

Closed tripodsan closed 2 years ago

tripodsan commented 2 years ago

Initial checklist

Affected packages and versions

rehype-minify-whitespace@5.0.0

Link to runnable example

https://codesandbox.io/s/eloquent-noether-zkw11y?file=/src/index.js

Steps to reproduce

create a html with an inline script with:

<script>
// comment
a=1;
</script>

and use rehype-minify-whitespace

Expected behavior

the whitespaces in the script tag should be folded correctly.

Actual behavior

the scripts are collapsed into a single line, but not accounted for line comments.

<script>// comment a=1;</script>

Runtime

Node v14

Package manager

npm 8

OS

macOS

Build and bundle tools

No response

wooorm commented 2 years ago

Nice catch. The algorithm currently only considers how text is presented on screen (say, when the script had display:block): https://github.com/rehypejs/rehype-minify/blob/fb62ec38ae7b22a9f76a9751198109eb0366ff6b/packages/rehype-minify-whitespace/index.js. But we should probably consider: https://github.com/rehypejs/rehype-minify/blob/fb62ec38ae7b22a9f76a9751198109eb0366ff6b/packages/html-whitespace-sensitive-tag-names/index.js#L28

Is this something you could help with?

github-actions[bot] commented 2 years ago

Hi! This was marked as ready to be worked on! Note that while this is ready to be worked on, nothing is said about priority: it may take a while for this to be solved.

Is this something you can and want to work on?

Team: please use the area/* (to describe the scope of the change), platform/* (if this is related to a specific one), and semver/* and type/* labels to annotate this. If this is first-timers friendly, add good first issue and if this could use help, add help wanted.

github-actions[bot] commented 2 years ago

Hi! This was closed. Team: If this was fixed, please add phase/solved. Otherwise, please add one of the no/* labels.

wooorm commented 2 years ago

released, thanks! https://github.com/rehypejs/rehype-minify/releases/tag/rehype-minify-whitespace%405.0.1