Automattic / juice

Juice inlines CSS stylesheets into your HTML source.
MIT License
3.12k stars 221 forks source link

Bump dependencies & node versions #497

Closed CarsonF closed 2 months ago

CarsonF commented 2 months ago

Needing release with web-resource-inliner v7 which fixes warning in Node 21+ Straight forward bumps Dropped support for Node 16-

I wanted to bump cheerio too to their 1.0 release published two weeks ago, but it had some regressions with doctype & meta tags that I didn't want to dig into. Still could be handled in another go with a minor release.

cossssmin commented 2 months ago

I wanted to bump cheerio too to their 1.0 release published two weeks ago, but it had some regressions with doctype & meta tags that I didn't want to dig into.

They changed the way you enable/configure htmlparser2 which is what we use, it's now done through the xml property:

https://cheerio.js.org/docs/advanced/configuring-cheerio#using-htmlparser2-for-html

Might be enough to just enable it instead of through useHtmlParser2: true which is what we do now:

https://github.com/Automattic/juice/blob/8caaf8fe4972af200a2abd5bd4a183897afe3b0f/lib/cheerio.js#L10

CarsonF commented 2 months ago

Alright I bumped it. It looks like it was that simple swap. The tests are failing with minor closing tags changes. Wanted to confirm this is expected/good before updating assertions.

CarsonF commented 2 months ago

Looking for workflow approval to see test failures.

Nevermind, I was wrong with these / fixed in last commit Difference examples: ```diff - + ``` ```diff -

+

jrit commented 2 months ago

There have been some users in the past that did not like the tag rewriting and needed the self closing tags preserved. I think if the behavior is changed there as least needs to be an option for the existing self closing tags or it'll get flagged as a bug by someone that needs it the other way. Keep in mind that some people are legit not rewriting html and are in other markup languages where it actually matters.

CarsonF commented 2 months ago

Ok actually it looks like those inconsistencies were fixed in my last commit.

The only failure now is with the document test. Given:

<style>div{color:red;}</style><div/>

Failed expectation:

<div style="color: red;"></div>

If the input div is self closed, why are we asserting here that it has a separate closing tag?

Also is the goal to avoid converting to self closing tags or avoid converting separate closing tags to self closed?

And also the test above it does the same thing! https://github.com/Automattic/juice/blob/master/test/run.js#L25-L26

jrit commented 2 months ago

will go as a major version bump due to the node version requirement change