maximize back compat

[ljharb]

This PR switches from jest to tape, so that we aren't limited by our test runner's engine support.

It also switches dequal (which requires node 6+) to deep-equal-json, which has minimal dependencies and so should still meet the constraints that motivated switching to dequal in the first place.

It also expands the test matrix so that all supported engines are tested, now and moving forward.

[benmccann]

It also switches dequal (which requires node 6+) to deep-equal-json, which has minimal dependencies and so should still meet the constraints that motivated switching to dequal in the first place.

I really don't think that's accurate. This new library introduces 16 additional dependencies: https://npmgraph.js.org/?q=deep-equal-json

Perhaps we can find a library with no additional dependencies that fits your requirements for Node 4 support?

[benmccann]

what does this do that's different from the standard actions/setup-node?

[ljharb]

It supports a wider range of node versions, and handles a lot of edge cases for npm installing in older ones. It also uses nvm to install node, and nvm install-latest-npm, which is the only way I'm aware of to reliably get the latest possible working version of npm on older nodes.

[gm112]

Easy way to sneak a backdoor in lol

[ljharb]

Indeed, it doesn't have as few dependencies, but it has fewer than deep-equal which was used previously. If it can be done with fewer dependencies and the same reliability and compat, then I've love to review some PRs on deep-equal-json (and deep-equal).

This PR extends support down to node 0.4 and equivalent browsers, which is useful because accessibility means including everyone, even those few users on old browsers.

[benmccann]

Can you show me where dequal would not work on an old browser?

[ljharb]

Fair, I hadn't yet looked at its code, just its engines declaration (which is important).

Some initial thoughts: the "reliability" part includes a lot of things that https://github.com/lukeed/dequal/blob/master/src/lite.js doesn't cover - delete Date.prototype.getTime after the module is imported but before it runs would crash it, eg, and the .constructor property is forgeable (can be faked, or deleted) and doesn't work across realms (iframes, node's vm module, etc).

[benmccann]

delete Date.prototype.getTime

Frankly, if you do that, you get what you deserve. I'd consider that breakage a feature rather than a bug as people should be prevented from engaging in such nonsense.

[Rich-Harris]

Some initial thoughts: the "reliability" part includes a lot of things that https://github.com/lukeed/dequal/blob/master/src/lite.js doesn't cover

As we established previously, this simply isn't relevant here: A11yance/aria-query#497 (comment)

As a user of this library I am strongly opposed to replacing dequal with a mess of dependencies that will bloat everyone's node_modules for absolutely no good reason whatsoever

[ljharb]

It's not always just "you" - extensions, user code, etc all can be running in an application. Also, the person who suffers isn't the person who wrote that code - it's the end user who suffers, and they did nothing wrong.

For what it's worth I hear you about wanting to minimize install/bundle size (I assume that's the metric that matters, since i can't figure out how dependency count would matter beyond those things), but I think what I'm not understanding is how correctness, robustness, reliability, and accessibility should all take a backseat to that constraint.

[ljharb]

@Rich-Harris if @lukeed is willing to expand the engines.node declaration (and testing matrix) to cover what's needed here, then that'd be fine. Since deep-equal-json already exists, this seemed more expedient.

[Rich-Harris]

what's needed here

No-one needs Node 4 support. This is utterly absurd.

[ljharb]

I understand you two feel it's absurd - you're not alone in feeling that way. That doesn't mean it's not a requirement. (do note that node 4 still has many millions of downloads according to node's own access logs)

[benmccann]

It's not always just "you" - extensions, user code, etc all can be running in an application. Also, the person who suffers isn't the person who wrote that code - it's the end user who suffers, and they did nothing wrong.

An extension that does that will immediately break everything. It will thusly have near zero users and does not need to be a consideration

For what it's worth I hear you about wanting to minimize install/bundle size (I assume that's the metric that matters, since i can't figure out how dependency count would matter beyond those things)

Dependency count actually matters greatly to us. https://learn.svelte.dev/ installs this version into a web container in the browser. 16 extra dependencies is an extra 16 HTTP calls.

I understand you two feel it's absurd - you're not alone in feeling that way. That doesn't mean it's not a requirement. (do note that node 4 still has many millions of downloads according to node's own access logs)

Engineering is about trade-offs. Version 3.2.0 was released without Node 4 support over a year ago. Not a single person has filed an issue during all that time.

Since 3.2.0 was released, a new major version of this library was released. If this change were going to be made it should at least be done as a backport to version 3 and we just update the changelog for version for to explicitly state that support for Node 4 has been dropped. But even that seems unnecessary to me

[lukeed]

Please stop

[ljharb]

@benmccann that might be an alternative path - to keep dequal, tighten up the engines.node on v4, and then do a backport to v3 that switches to an alternative dependency, since eslint-plugin-jsx-a11y isn't on v4 yet - your belief that nobody will care about node 4 may allow for the breaking change within v4 of restricting engines.node.

Engineering is indeed about tradeoffs, and I clearly think that dep count and install/bundle size are a worthy sacrifice in the face of compatibility, accessibility, correctness, and reliability.

@lukeed "Please stop" is not a constructive contribution to the discussion, but I'll take that to mean that you're not going to make any changes to dequal.

[ljharb]

Dependency count actually matters greatly to us. https://learn.svelte.dev/ installs this version into a web container in the browser. 16 extra dependencies is an extra 16 HTTP calls.

it seems like it'd make a lot of sense then to have a webserver to load and cache all the code you need, so that the user's browser doesn't need to do the installs themselves - especially since many users, including in China, can't access npm directly. I assume that's not a new suggestion, though, and there's reasons you went with this approach.

[Rich-Harris]

Engineering is indeed about tradeoffs, and I clearly think that dep count and install/bundle size are a worthy sacrifice in the face of compatibility, accessibility, correctness, and reliability.

Once again, since this point seems to have been missed: as far as this library is concerned, deep-equal-json has no advantages over dequal in this regard.

it seems like it'd make a lot of sense then to have a webserver to load and cache all the code you need

FWIW we actually do do this, the dependency is among others bundled into a zip file. But that's immaterial — in the common scenario, adding bloat like this does affect users.

Not a single person has filed an issue during all that time.

Which is the crux of the matter. Candidly, you are wasting everyone's time and bandwidth, not to mention this project's quota of actions minutes.

[codecov-commenter]

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

[KonnorRogers]

why is this needed?

if people need support for older Node versions, they can add a polyfill to support the missing APIs.

[ljharb]

@Rich-Harris Public projects have no action minutes quotas, and to me this is a good use of my time.

@benmccann trying to summon a twitter mob isn't very professional or constructive, if I need to lock the thread then none of you will have any further chance to convince me of anything here.

[Rich-Harris]

You're pretending I said something I didn't.

[ljharb]

@Rich-Harris youre being very careful to not directly accuse of me anything - but bringing money into the discussion IS an accusation. It’s simply irrelevant. I’d be doing the exact same things even if it cost me money to do them - my motives are pure.

@valadaptive i live in the SF Bay Area, and i have children. It is in fact insignificant supplemental income; if i was grubbing for money I’d spend the time contracting and make 10+ times that. (also, package income simply doesn’t scale that way - tidelift doesn’t have enough subscribers for that)

[valadaptive]

someone brought up a really important issue here https://x.com/samgoodwin89/status/1804577766232437005

I looked at this briefly, and right now I don't think there's any way to inject a backdoor via compromising the GitHub action. AFAIK, there are no NPM credentials stored in this repo and no way to publish via CI--it's only used for running tests.

[valadaptive]

I can appreciate how awestruck everyone is, but I can't help but worry that "omg who did this??? 😂😂😂" type comments will cause this discussion to be locked, which is a shame since there is a lot of real discussion about the JS ecosystem going on here.