search

jshttp / http-assert

assert with status codes
MIT License
151 stars 14 forks source link

chore!: replace deep-equal with dequal #14

Closed wojtekmaj closed 1 year ago

wojtekmaj commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

wojtekmaj commented 1 year ago

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 commented 1 year ago

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.