cheerio
cheerio copied to clipboard
cheeriojs
rc.4 includes an implicit Node bump
When I updated to rc5, tests started failing in node 4: https://travis-ci.com/github/enzymejs/enzyme/builds/210326410
Specifically, parse5-htmlparser2-tree-adapter is using object destructuring syntax, which means including it is a breaking change :-/
Thanks for flagging! For starters, it is unfortunate that this wasn't caught earlier. The CI should cover the oldest supported Node version, which it didn't do here. Sorry about that!
It seems like parse5 doesn't support node@4 anymore. This also applies to eg. the serializer, which uses class syntax. Same for files used by the parser.
I'm inclined to bump the minimal version requirement of cheerio, as not using parse5 is not really an option at this point. Given that we are about to release a major version, this would be the point in time to do it. And when we do the bump, we might just go to the current Node LTS version, which is 10.
@ljharb I'd love to hear your thoughts here. I know this is an inconvenience for enzyme, and I'd love find a good solution for both projects.
@fb55 rc releases should've probably be published with --tag=next @ljharb It's very sad we have to have bloated node_modules because of many years dead node versions in all your projects
rc releases should've probably be published with --tag=next
Agreed. Unfortunately, changing this now will leave people stuck on one of the older RCs, as well as promote an old version that features many issues solved in more recent releases. Also see #1261.
It's very sad we have to have bloated node_modules because of many years dead node versions in all your projects
That's a bit harsh. There is a lot of value in supporting as wide of a user base as possible, and the success these projects have speaks for itself. That's why I also don't want to be too aggressive with cheerio.
I'd love to maintain backwards compatibility to Node 4, but don't see a great way forward given that we depend on parse5.
One more involved way would be to separate parsers/serializers out from cheerio. @matthewmueller proposed something similar in https://github.com/cheeriojs/cheerio/discussions/1271#discussioncomment-207120. We could move parsers & serializers to the options, allowing users with older systems (or size limits) to use htmlparser2.
@TrySound Testing frameworks in particular need to be able to test in all supported environments, which includes browsers for which older node versions are a good substitute. node_modules size is unimportant; supporting users is quite important.
@fb55 is the use of syntax the only reason those packages don’t work on node 4? All of those could be run through babel and they’d still work fine. Perhaps if the maintainers of those packages refuse to do so, cheerio could maintain forks of them that basically just add a babel run on top?
In the meantime, try/catch requiring of the updated deps, and falling back to htmlparser2, seems like it would fix the bugs for modern engines while restoring rc3 behavior for older ones?
In the meantime, try/catch requiring of the updated deps, and falling back to htmlparser2, seems like it would fix the bugs for modern engines while restoring rc3 behavior for older ones?
The issue here would be a change in behavior for different versions of node. Especially when writing tests, that seems undesirable.
All of those could be run through babel and they’d still work fine.
That is true, but the perspective of maintaining a fork of the module with all files passed through babel isn't particularly exciting. I will think about this some more, there might be some automation that can save the day.
@fb55 there's another problem - cheerio-select-tmp has a peerDep on @types/node@^14.11.2, which is both a breaking change, and an unreasonable burden to impose on non-TS users. DT packages should only be dev deps.
cheerio-select-tmphas a peerDep on@types/node@^14.11.2
Whee, good catch. This isn't actually used, I've removed it in https://github.com/cheeriojs/cheerio-select/commit/9e002c64119e6e8d0a200b3e7ccda5fb6bef9a84
Another issue that should be tackled IMHO is that the node property in package.json is >= 0.12. I doubt the current code works before Node.js 4. At least Object.assign is used which isn't available AFAICT before Node.js 6.
Luckily API methods are easier to fix than syntax - https://npmjs.com/object.assign for example
Right, I meant to write v4. Still, 0.12 is wrong AFAICT.
On Sun, Dec 27, 2020, 17:53 5saviahv [email protected] wrote:
Object.assign https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign#Browser_compatibility seems to be added on 4.0.0
I tested on v4.9.1 where it works
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/cheeriojs/cheerio/issues/1585#issuecomment-751483527, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACVLNNRMP3UNPKLGG3HJ2DSW5J6PANCNFSM4VGGHC6Q .
I agree v4 should be minimum. Between v0.12 and v4, node was actually split in node.js and io.js
Technically it was between v0.10 and v4, and v0.12 came out during io.js; but i don’t think that’s at all relevant.
it’s still a breaking change for enzyme if 0.12 doesn’t work, fwiw.
Might be worth trying https://github.com/mysticatea/eslint-plugin-node?
Since we can't test versions < 10 versions on CI, this might help.
And why can't we? I test 200 versions of node on CI on all my packages - every major/minor.
Because of syntax errors?
On Mon, Dec 28, 2020, 17:56 Jordan Harband [email protected] wrote:
And why can't we? I test 200 versions of node on CI on all my packages - every major/minor.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/cheeriojs/cheerio/issues/1585#issuecomment-751761732, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACVLNI63VXZPQQ45ZLUPF3SXCTCRANCNFSM4VGGHC6Q .
The tests would reveal those syntax errors - we'd test on every supported node version, allow failure on the ones that aren't currently working, and then fix those.
Technically any breaking change can happened in any rc version. Dependent projects should consider this when using non-semver. Forgotten "engines" is just a bug here.
@TrySound while this is true, v1 has been in rc for years, and enzyme relies on it. Just because it's technically allowed doesn't mean it's a good idea, or that it was intentional :-)
I used babel with parse5 and tested with rc.5 and node 4 and it worked.
Only domutil seems to use Array.includes() what was not in node 4, but polyfill did the job.
I also ran all cheerio tests with babeled parse5 and they passed.
The ideal would be convincing parse5 to include babel themselves (and https://npmjs.com/array-includes in domutil)
I would suggest to stick with indexOf instead caz this package is monstrous https://packagephobia.com/[email protected]
indexOf is fine, but package size doesn’t matter, and none of these tools are bundled.
Doesn't matter to You. I worry when useless (i dont use insecure -> dangerous environments) packages bloat my disk.
@5saviahv that's fine for testing things out, but it modifies the global, so it's a nonstarter for a package to use it.
@TrySound no need to have that debate here, but the important thing is correctness. Let's ship the proper fix, and worry about optimizing disk space usage later.