Switch to dequal to remove 45 transitive dependencies

[benmccann]

deep-equal has 45 transitive dependencies whereas dequal has none. This is important for Svelte which loads axobject-query in the browser as part of https://svelte.dev/repl

See also A11yance/axobject-query#311

[ljharb]

Why can't svelte bundle?

[ljharb]

Also, this isn't equivalent; dequal relies on the runtime (as opposed to "require time") existence of lots of builtins, which makes it brittle in an environment that runs untrusted code (namely, all environments). On the other hand, deep-equal shouldn't be relying on virtually anything at runtime, only at "require time". Also, deep-equal works across realms (iframes); dequal does not.

[benmccann]

Even with bundling, there's absolutely no reason to add 45 extra dependencies for such a simple piece of functionality. It still adds an extra 91k of output after bundling not to mention opening users up to more supply chain attacks by adding 45 extra chances for a package to be compromised. It also forces users to apply workarounds because some of the dependencies of deep-equal use process.env, which is not defined in the browser and many bundlers (including Rollup, which Svelte uses) don't polyfill process.env by default.

dequal relies on the runtime (as opposed to "require time") existence of lots of builtins

You can see the implementation here. It's tiny and the function I'm calling doesn't require any builtins: https://github.com/lukeed/dequal/blob/master/src/lite.js. deep-equal on the otherhand does require builtins via its transitive dependencies, which causes us some pain and makes us jump through hoops to ship it to the browser and is another reason I'm proposing to swap it out

Also, deep-equal works across realms (iframes); dequal does not.

I'm afraid I have no idea what you're talking about. How does checking if two javascript objects are equal involve iframes and how would that be relevant to the aria-query library?

[ljharb]

Um, it's calling methods on prototypes, so it does rely on builtins - .getTime() on dates, .toString() on regexes, etc.

[ljharb]

I'm afraid I have no idea what you're talking about. How does checking if two javascript objects are equal involve iframes and how would that be relevant to the aria-query library?

This line will be false if the Date comes from an iframe, as will any lines that compare constructors by identity, since that is a brittle and broken approach. (also, any reliance on .constructor is also brittle, since there's no guarantee that property will exist at the time the code runs)

[benmccann]

Ah, I was talking about Node built-in modules. E.g. deep-equal transitively depends on the Node util package, which requires it to be polyfilled in the browser

If users are going around redefining methods on JavaScript objects in ways that break them, they're going to run into trouble irrespective of what we do here and it doesn't seem like the solution should be for projects to avoid calling any methods that JavaScript provides by default

[ljharb]

and yet, deep-equal, like most of my projects, are robust against later modification of builtins. That might not be something you care about, but it's something that's important, since untrusted code often runs in every environment.

[dummdidumm]

None of this applies here. These are simple objects only containing primitives like strings and numbers (no dates) and they are all under control of this repository. If these checks fail because you patch object prototypes in ways that break it you have bigger problems than a failed comparator function in one of your dependencies. dequal is the right tool for the right job here.

[benmccann]

@jessebeach any chance you would be able to take a look at this? Nearly all of Svelte's dependencies are coming from the deep-equal package. We just released 4.0.0-next.0 and would love to be able to promote that we're using an enhanced version of aria-query as part of the 4.0 final release in a couple of weeks.

From https://arve0.github.io/npm-download-size/#svelte@4.0.0-next.0:

[jessebeach]

any chance you would be able to take a look at this?

@benmccann point made :) This is not a list I want the #1 and #2 spots on. I appreciate you taking the time to show the impact here.

What is your ideal timeline? Finding time to code these days is a little difficult with a tiny human demanding my attention at home =D

[benmccann]

Thanks so much for taking the time to respond @jessebeach! We don't have an exact date yet for the Svelte 4 release, but are planning for roughly two weeks from now. This wouldn't be a breaking change so we could always do it in a minor release later on, but it would be fun to tout that https://svelte.dev/repl loads faster in the v4 release announcement.

If there's anything you'd like to check feel free to let me know and I'd be happy to do the leg work. I've got a couple little humans running around too, so know how much scarcer time becomes!

[ljharb]

Another alternative is that I could extract some subsets of checks out of deep-equal, and then aria-query could use those - and then the same back compat and robustness would be assured with far fewer deps.

If that's palatable, then an exhaustive list of the types of things checked here (plain objects, arrays, strings, numbers?) would be all I'd need.

[jessebeach]

If that's palatable, then an exhaustive list of the types of things checked here (plain objects, arrays, strings, numbers?) would be all I'd need.

Yes, plain objects, arrays, strings and numbers.

The usages are here:

aria-query/src/elementRoleMap.js

Line 27 in 1160138

const elementRoleRelation: ?ElementARIARoleRelationTuple = elementRoles.find(relation => deepEqual(relation, concept));

and here:

aria-query/src/elementRoleMap.js

Line 70 in 1160138

key.name === tuple[0].name && deepEqual(key.attributes, tuple[0].attributes)

We're comparing ARIARoleRelation types, defined here:

aria-query/flow/aria.js

Lines 352 to 355 in 1160138

	type ARIARoleRelation = {|
	module?: string,
	concept?: ARIARoleRelationConcept,
	|};

The ARIARoleRelationConcept

aria-query/flow/aria.js

Lines 360 to 385 in 1160138

	type ARIARoleRelationConcept = {|
	name: string,
	attributes?: Array<ARIARoleRelationConceptAttribute>,
	// These constraints are drawn from the mapping between ARIA and HTML:
	// https://www.w3.org/TR/html-aria
	constraints?: Array<
	| 'scoped to the body element'
	| 'scoped to the main element'
	| 'scoped to a sectioning root element other than body'
	| 'scoped to a sectioning content element'
	| 'direct descendant of document'
	| 'direct descendant of ol'
	| 'direct descendant of ul'
	| 'direct descendant of menu'
	| 'direct descendant of details element with the open attribute defined'
	| 'ancestor table element has table role'
	| 'ancestor table element has grid role'
	| 'ancestor table element has treegrid role'
	| 'the progress bar is determinate'
	| 'the datalist selection model allows multiple option elements to be selected at a time'
	| 'the aria-controls attribute is set to the same value as the list attribute'
	| 'the size attribute value is greater than 1'
	| 'the multiple attribute value is greater than 1'
	| 'the multiple attribute and the size attribute do not have a value greater than 1'
	| 'the list attribute is not set'>,
	|};

And the ARIARoleRelationConceptAttribute

aria-query/flow/aria.js

Lines 387 to 392 in 1160138

	type ARIARoleRelationConceptAttribute = {|
	name: string,
	value?: string | number,
	// These constraints are drawn from the mapping between ARIA and HTML:
	// https://www.w3.org/TR/html-aria
	|};

Here are some juicy examples

https://github.com/A11yance/aria-query/blob/main/scripts/roles.json#L619-L769

[ljharb]

Would it still need to handle cycles, or boxed primitives? Would you use loose mode, or strict mode, or both?

[jessebeach]

Would it still need to handle cycles, or boxed primitives? Would you use loose mode, or strict mode, or both?

In this case, I need value equality, and not instance equality. I don't know how that translates to loose or strict. But to give an example:

let obj1 = {
  "name": "aria-valuemin",
  "value": 0,
};

let obj2 = {
  "name": "aria-valuemin",
  "value": 0,
};

I can't do obj1 === obj2 because the objects are separately defined (the example is trite. In the plugin, the objects are defined in different files), so the properties and values must compared. And they must be exact. So if all the properties overlap, but obj2 has an extra property that obj1 does not have, then the comparison fails.

As for cycles, The objects don't contain references to ancestor nodes in the same tree. A DFS or BFS would work.

[ljharb]

"loose" would mean that 0 and '0' are considered the same.

[jessebeach]

"loose" would mean that 0 and '0' are considered the same.

Aha! No, I have absolute control over the data in this project. I would rather update the data to be a number or string as it should be, than have to rely on loose matching.

0 == "0" // false
"0" == "0" // true
0 == 0 // true

[ljharb]

https://npmjs.com/deep-equal-json

@benmccann is this a sufficient improvement for the metrics you care about?

[benmccann]

Thanks for making the effort. It's an improvement, but still has a ways to go in my opinion. dequal is 4k with 0 dependencies and deep-equal-json is 67k with 11 dependencies

https://arve0.github.io/npm-download-size/#dequal

https://arve0.github.io/npm-download-size/#deep-equal-json

Is there any reason not to just merge this PR as is? I don't see the need to create new libraries when we have a fully working solution right here already

[ljharb]

It's not equivalent because it's not robust against later builtin modification.

I can certainly remove object-is, though, if that would tip the balance for you.

[ljharb]

More to the point, dep count and package size are largely irrelevant metrics when compared to correctness, so I'd hope that the latter wouldn't be sacrificed to appease the former.

[benmccann]

Would you be happier to use fast-deep-equal than dequal? It's also dependency-free and actually even slightly smaller than dequal

Protection against built-in modification seems irrelevant to aria-query and to most projects. There are dozens or possibly hundreds of usages of built-in methods within aria-query itself, so what good does it do to choose a dependency that avoids built-in modifications? It's simply not a goal of the aria-query project as far as I can tell - though please correct me if I'm wrong about this @jessebeach

It would be nearly impossible to write any decently-sized JavaScript project without using any built-in methods. To say that a JavaScript library cannot use any method on an object provided by the language is an incomprehensibly insane position to take. While it's fine if you want to do that for your own projects, it seems absolutely mad to try to force it on the rest of the ecosystem especially when it comes at a cost of making libraries 15x larger. The easier position to take would be that the extremely rare projects that override built-in methods in a breaking way should be the ones to be changed

I also question how this policy could even be enforced. Are you regularly auditing every version of each of the 45 dependencies of deep-equal to ensure that they don't use any built-in methods? It seems impossible to do by hand and so there would need to be some kind of automated check for it. If we really wanted to avoid built-in methods in aria-query then we'd need such a check in its CI to enforce it. But again, I don't think this insanity is worth exploding the size of every library in the JS ecosystem to allow the bad practice of modifying built-in methods to continue

[ljharb]

Yes, I am, but that's easy because I maintain all of them :-) Basically all of my 400+ packages follow this principle.

[Rich-Harris]

Please correct me if I've misunderstood the conversation (I've been looking at the source code for deep-equal and its dependencies, and there's a lot of indirection, so I could be wrong here):

The argument is that deep-equal protects against cases where someone has redefined prototype methods like Date.prototype.getTime. To compare two dates, instead of doing this...

if (a.getTime() !== b.getTime()) return false;

...it does this:

if ($getTime(a) !== $getTime(b)) return false;

Here, $getTime is callBound('Date.prototype.getTime').

But as far as I can tell, callBound only protects against this behaviour if it is called before the prototype is messed with:

Date.prototype.getTime = () => 'lol';

const { default: CallBound } = await import('call-bind/callBound');
const $getTime = CallBound('Date.prototype.getTime');

console.log($getTime(new Date()); // 'lol'

So — again, please correct me if I'm mistaken — this doesn't protect against built-in modification (and if it did, it wouldn't do so for the rest of your app), it only mitigates it, at signficant cost. This seems like a very bad trade-off that results in slower/more bloated apps in return for a false sense of security.

[benmccann]

I'd like to re-iterate that considering whether built-ins are used for Date and Regex is completely irrelevant anyway. As both @dummdidumm (#497 (comment)) and @jessebeach (#497 (comment)) have pointed out, the comparisons being done here are on plain objects, arrays, strings and numbers. Thus, the fact that dequal uses built-ins for Date and Regex has no bearing even in the extremely unlikely scenario that these methods have been overridden.

[ljharb]

@Rich-Harris yep, that's right - it's impossible in JS to defend against first-run code, but what my packages do is defend against second-run code - ie, code that's evaluated after mine has cached the method, which is a common security approach used by many environments, including Salesforce, Runkit, and a number of others.

[Rich-Harris]

If guarding against that sort of thing is desirable in those environments, it would be far more effective for them to do something like this before importing anything:

Object.freeze(Date.prototype); // and everything else

Expecting everyone to avoid built-ins is very silly indeed.

[ljharb]

You're welcome to believe so, but I think it's critical to do so, especially in packages that are heavily transitively used.

[Rich-Harris]

Even if that were true, it's simply not relevant in the case of this library — as articulated above, the only things being compared are numbers, strings, plain objects and arrays. As far as I can tell, the only time dequal will call built-ins when dealing with these values is on this line...

return Object.keys(bar).length === len;

...and if someone has overwritten Object.keys with a function that returns something other than an array of its argument's keys, then you have much bigger problems. Even that could be trivially mitigated (to the same extent that get-intrinsic is able to mitigate against this stuff, which is to say... partially) by putting const { keys } = Object at the top of the module and using keys instead of Object.keys.

For the purposes of this library, dequal is categorically a better choice.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit f28e865:

Sandbox	Source
Vanilla	Configuration

[jessebeach]

I appreciate the debate y'all. I'll get a release cut this week. It's past 9pm where I am and the baby does not respect late morning wake-ups :)

[benmccann]

Thank you so much @jessebeach!! I really appreciate it!

Btw, just so it doesn't fall off the radar, I sent the same change to axobject-query, which we'll need as well: A11yance/axobject-query#311

[jessebeach]

Btw, just so it doesn't fall off the radar, I sent the same change to axobject-query, which we'll need as well: A11yance/axobject-query#311

Thank you 🙏 . I landed that one as well.

[benmccann]

@jessebeach just a gentle reminder about a new release. thanks!

[jessebeach]

Thank you for the reminder! It's on my mind. Just a few things in my personal life to attend to this weekend. I'll have time this week.

[jessebeach]

@benmccann et voilà.

aria-query@5.2.0: https://www.npmjs.com/package/aria-query
axobject-query@3.2.0: https://www.npmjs.com/package/axobject-query

[ljharb]

Looks like there's a missing call site → #502

[gtm-nayan]

It's used on this line in the file on npm, but it's been already converted. The only reference to deep-equal I can find in this repo now is in a flow definitions file.

Edit: forgot to add the line

aria-query/src/elementRoleMap.js

Line 27 in ef150a3

const elementRoleRelation: ?ElementARIARoleRelationTuple = elementRoles.find(relation => dequal(relation, concept));

Building it locally gives me the correct "dequal" import

[jessebeach]

Should be fixed now. I had to run nom build locally before running npm publish, which I've never had to do before. In the past, npm would build as part of the publish step. Something must have changed with Github Actions, so I'll need to update the publish step so that this doesn't happen again in the future. Sorry for the thrash!

[benmccann]

thank you so much!! we just updated the Svelte 4 branch with the new version. really appreciate your help!

[benmccann]

Here's our release announcement: https://svelte.dev/blog/svelte-4. Thanks again @jessebeach!

[ljharb]

Turns out this PR was a breaking change (to axobject-query as well) - dequal declares an engines.node of >= 6, but aria-query 5 and axobject-query 3 have no engines declaration (which means they support everything).

If you're interested in fixing it, either this PR will need to be reverted, or dequal will need to extend its engines.node support significantly.

[benmccann]

Seeing as how this has been released for over a year and no one has noticed, I don't think there's a need to do anything. Node 6 reached EOL five years ago and is still supported by this PR. That's plenty old enough

[ljharb]

That's certainly a position one can take, but that's not how semver works - a breaking change is a breaking change.

[benmccann]

If we want to be pedantic about semver, the most reasonable solution would be to release a new major. And in that case I'd add an engines field that drops everything that is EOL and add a CI matrix that tests every supported version to avoid inadvertent breakages

[ljharb]

Yes, that's exactly what I'd suggest - v6 can keep using dequal and have as aggressive an engines requirement as desired, and v5 can get a revert.

However, node's EOL status is irrelevant - old node versions are helpful stand-ins for the older browsers that need accessibility ensured the most. (which, to be fair, doesn’t really apply to linting, but definitely applies to this library and its in-browser usage)

[jozefizso]

... old node versions are helpful stand-ins for the older browsers that need accessibility ensured the most.

Which browsers are those?

[marcysutton]

@ljharb we chatted in person but can you add some info about in-browser usage? Which tools are using it in-browser, and how?

[ljharb]

@marcysutton in this particular case, the only in-browser usage I'm aware of is svelte's, which presumably only includes modern browsers. My response was about the general case to explain why my packages broadly support super old node.

[marcysutton]

@ljharb gotcha, thanks for the update. I think it's worth it to be pragmatic about how the library is used when making those decisions, especially since community feedback has shown that consumers are concerned about bundle size and maintenance. Older browsers are not going to have the same level of ARIA support as modern ones, especially for features in the newer specs. So when considering these trade-offs of maintenance and support, I think you'll have to be more specific about "Node as a helpful stand-in" for an old browser. Which ARIA attributes/APIs are at play in those environments, if at all? It might help to answer the question of relevancy for these libraries in particular.

[ljharb]

Unfortunately these threads tend to conflate general ecosystem-wide concerns with aria-query-specific concerns :-)

Specifically, aria-query and axobject-query quite certainly don't need to have support for older browsers or nodes, for the reasons indicated. The only consideration for v3 is that dropping any engine for any reason is semver-major.

For v4, it could have been released with engines.node set to >= 22 if that's what's desired, and that'd be perfectly fine. As it is now, v4 must support 6+, so to restrict it further, a v5 would be needed - which would also be fine.