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

Bump dependencies & node versions #497

Merged
merged 5 commits into from
Aug 22, 2024
Merged

Bump dependencies & node versions #497

merged 5 commits into from
Aug 22, 2024

Conversation

CarsonF
Copy link
Contributor

@CarsonF CarsonF commented Aug 21, 2024

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.

package.json Outdated Show resolved Hide resolved
@cossssmin
Copy link
Collaborator

cossssmin commented Aug 21, 2024

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:

options = Object.assign({decodeEntities: false, _useHtmlParser2:true}, options);

@CarsonF
Copy link
Contributor Author

CarsonF commented Aug 21, 2024

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
Copy link
Contributor Author

CarsonF commented Aug 21, 2024

Looking for workflow approval to see test failures.

Nevermind, I was wrong with these / fixed in last commit

Difference examples:

- <meta name="viewport" content="width=device-width">
+ <meta name="viewport" content="width=device-width"/>
- <p class="text"></p>
+ <p class="text"/>

</details>

@jrit
Copy link
Collaborator

jrit commented Aug 22, 2024

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
Copy link
Contributor Author

CarsonF commented Aug 22, 2024

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 jrit merged commit 0a0b026 into Automattic:master Aug 22, 2024
3 checks passed
@jrit
Copy link
Collaborator

jrit commented Aug 22, 2024

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

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

Successfully merging this pull request may close these issues.

4 participants