-
Notifications
You must be signed in to change notification settings - Fork 109
refactor: replace lodash.isequal with node:util.isDeepStrictEqual, #1606 #1809
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
base: main
Are you sure you want to change the base?
refactor: replace lodash.isequal with node:util.isDeepStrictEqual, #1606 #1809
Conversation
bugarela
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for taking this on and sorry for the late review!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not delete this file, it is necessary for building Quint with nix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yazaldefilimone can you add this file back please? We can't delete it.
quint/src/util.ts
Outdated
| * | ||
| */ | ||
| export function isEqual<L, R>(left: L, right: R): boolean { | ||
| return isDeepStrictEqual(left, right) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we need this indirection here through util - we could just replace the imports to import isDeepStrictEqual and then use it directly instead of isEqual everywhere. @erickpintor do you have some advice here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. It'd be simpler to just rely on node:util for equality everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yazaldefilimone can we delete this now?
Co-authored-by: Gabriela Moreira <gabrielamoreira05@gmail.com>
Co-authored-by: Gabriela Moreira <gabrielamoreira05@gmail.com>
Co-authored-by: Gabriela Moreira <gabrielamoreira05@gmail.com>
Co-authored-by: Gabriela Moreira <gabrielamoreira05@gmail.com>
bugarela
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Sorry, I don't get notified by commits so you need to mention me or re-request a review when you want me to take a look again. We just have two more small things and then are good to merge
quint/src/util.ts
Outdated
| * | ||
| */ | ||
| export function isEqual<L, R>(left: L, right: R): boolean { | ||
| return isDeepStrictEqual(left, right) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yazaldefilimone can we delete this now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yazaldefilimone can you add this file back please? We can't delete it.
…ilimone/quint into refactor/replace-lodash-isequal
bugarela
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
quint/src/effects/simplification.ts
Outdated
|
|
||
| import isEqual from 'lodash.isequal' | ||
| import { unreachable } from '../util' | ||
| import { isEqual, unreachable } from '../util' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is making us fail to compile
| import { isEqual, unreachable } from '../util' | |
| import { unreachable } from '../util' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And we also need to add an import for isDeepStrictEqual
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bugarela sorry... fix now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the codebase to replace the external lodash.isequal package with Node.js's built-in node:util.isDeepStrictEqual function, reducing external dependencies.
- Removes the
lodash.isequaldependency from package.json and package-lock.json - Updates function calls from
isEqualtoisDeepStrictEqualacross multiple files - Updates documentation in CONTRIBUTING.md to reflect the new equality comparison approach
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| quint/src/util.ts | Updates documentation comment example (contains error) |
| quint/src/types/substitutions.ts | Changes equality check in type substitution (missing import) |
| quint/src/runtime/impl/nondet.ts | Updates comparison in nondeterministic evaluation loop (missing import) |
| quint/src/effects/substitutions.ts | Changes equality check in effect substitution (missing import) |
| quint/src/effects/simplification.ts | Updates entity deduplication logic and import (incorrect import) |
| quint/src/effects/modeChecker.ts | Adds import and updates entity comparison calls |
| quint/src/effects/base.ts | Adds import and updates multiple entity unification comparisons |
| quint/package.json | Removes lodash.isequal dependency |
| quint/package-lock.json | Removes lodash.isequal from lockfile |
| CONTRIBUTING.md | Updates documentation with new equality function and code examples |
Files not reviewed (1)
- quint/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * ``` | ||
| * const result = findMap([1,2,3], (x) => x % 2 === 0 ? just(x) : none<int>()) | ||
| * lodash.isEqual(result, just(2)) | ||
| * lodash.isDeepStrictEqual(result, just(2)) |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation comment incorrectly refers to lodash.isDeepStrictEqual. Since lodash doesn't have an isDeepStrictEqual method, this should just be isDeepStrictEqual (from node:util). Additionally, util.ts needs to import and re-export isDeepStrictEqual from node:util so that simplification.ts can import it from this module.
| */ | ||
|
|
||
| import isEqual from 'lodash.isequal' | ||
| import { unreachable } from '../util' |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import statement references isEqual from '../util', but this file doesn't export isEqual. The util.ts file needs to import isDeepStrictEqual from 'node:util' and either export it directly or re-export it as isEqual. Additionally, the usage at line 102 should be isDeepStrictEqual, not isEqual.
|
|
||
| const result = singleApplication() | ||
| if (isEqual(result, t)) { | ||
| if (isDeepStrictEqual(result, t)) { |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function call uses isDeepStrictEqual but this function is not imported in this file. The import statement needs to be added: import { isDeepStrictEqual } from 'node:util'. Additionally, the existing import { isEqual } from 'lodash' import should be removed if it's no longer used elsewhere in the file.
|
|
||
| // Retry if condition is satisfied and we haven't exhausted all possible positions. | ||
| } while (shouldRetry && !isEqual(newPositions, originalPositions)) | ||
| } while (shouldRetry && !isDeepStrictEqual(newPositions, originalPositions)) |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function call uses isDeepStrictEqual but this function is not imported in this file. The import statement needs to be added: import { isDeepStrictEqual } from 'node:util'. Additionally, the existing import { isEqual } from 'lodash' import should be removed if it's no longer used elsewhere in the file.
|
|
||
| return result.map(simplify).chain(r => { | ||
| if (!isEqual(r, e)) { | ||
| if (!isDeepStrictEqual(r, e)) { |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function call uses isDeepStrictEqual but this function is not imported in this file. The import statement needs to be added: import { isDeepStrictEqual } from 'node:util'. Additionally, the existing import { isEqual } from 'lodash' import should be removed if it's no longer used elsewhere in the file.
| */ | ||
|
|
||
| import isEqual from 'lodash.isequal' | ||
| import { unreachable } from '../util' |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import isEqual.
(including screenshots is helpful)
CHANGELOG.mdfor any new functionality