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

Replace fast-deep-equal with Node.js built-in #49

Closed
wants to merge 1 commit into from
Closed

Replace fast-deep-equal with Node.js built-in #49

wants to merge 1 commit into from

Conversation

jonahsnider
Copy link

Remove the fast-deep-equal dependency, replacing it with Node's built-in util library.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 9900405988

Details

  • 4 of 4 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 34.765%

Totals Coverage Status
Change from base Build 9848741379: 0.0%
Covered Lines: 1190
Relevant Lines: 3084

💛 - Coveralls

@hjdhjd
Copy link
Contributor

hjdhjd commented Jul 12, 2024

I would not recommend this change. While functionally identical, these are core tests that are key in ensuring ciao is highly performant. fast-deep-equal is a couple of orders of magnitude faster...and given how often these checks will occur, it's a real performance hit. 😄 Here's just one performance benchmark...but there are lots of others out there: https://blog.alexefimenko.com/posts/compare-objects

Comparing objects is hard stuff because the structures are so funky. We'd almost be better off creating a completely bespoke version for ciao for the use cases we need that might squeeze out a shade more performance...but that's a project for another day.

TL;DR: I would not recommend we do this for performance considerations unless we have data otherwise.

@jonahsnider jonahsnider deleted the node-deep-equal branch July 12, 2024 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants