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

chore!: replace deep-equal with dequal #14

Closed
wants to merge 1 commit into from

Conversation

wojtekmaj
Copy link

@wojtekmaj wojtekmaj commented Sep 25, 2023

Replaces bloated deep-equal module with much smaller alternative dequal.

Please note that dequal requires Node.js 6.0 so if Node 0.8 is indeed still supported, this should be considered a breaking change.

@dougwilson
Copy link
Contributor

Thank you. We don't need to support thos Node.js versions any longer. Are there any comparison behavior differences between the two? It is ok if they are, I just want to document them in the change notes.

@dougwilson dougwilson added the pr label Sep 25, 2023
@dougwilson
Copy link
Contributor

I took a look and unfortunately dequal is not a drop in replacement for deep-equal, as the equality is strict in dequal, which is not what Node.js deepEqual does. We can add a deepStrictEqual however, if you want strict deep equality, but that module unfortunately cannot be used for deepEqual.

@dougwilson dougwilson closed this Sep 25, 2023
@wojtekmaj
Copy link
Author

wojtekmaj commented Sep 25, 2023

Aw, snap. Tests were green, I was hoping it would be alright.

fast-deep-equal then, maybe? Super popular choice nowadays, >2x more popular than deep-equal in fact. Used in ajv, eslint…

If you would be kind enough to update the tests to better reflect what this package is trying to achieve, I'd be more than happy to try stuff out and raise another PR.

@dougwilson
Copy link
Contributor

Aw, snap. Tests were green, I was hoping it would be alright.

Yes, apologies. The tests are pretty bare bones 💀

fast-deep-equal then, maybe? Super popular choice nowadays, >2x more popular than deep-equal in fact. Used in ajv, eslint…

Off hand sounds good as long as non strict equality to match the Node.js deepEqual.

If you would be kind enough to update the tests to better reflect what this package is trying to achieve, I'd be more than happy to try stuff out and raise another PR.

Yea, no problem. I will look in to that when I get home tonight. I think I can just copy the Node.js tests for deepEqual into here.

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

Successfully merging this pull request may close these issues.

2 participants