fix(NODE-7436): EJSON.stringify type signature#870
Conversation
|
@chdanielmueller , thanks so much for your contribution! The team is going to prioritize this PR and NODE-7436 at our next triage session. |
tadjik1
left a comment
There was a problem hiding this comment.
hi @chdanielmueller, thanks a lot for the contribution. The bug is real, well-documented, and the test coverage you added is great.
The core issue is that space = 0 discards the user's space value when options are passed in the replacer position.
However, the fix is significantly more complex than it needs to be. The entire bug can be fixed by deleting one line (space = 0). The existing code already treats any non-array object in replacer position as options.
I would suggest minimal fix:
if (replacer != null && typeof replacer === 'object' && !Array.isArray(replacer)) {
options = replacer;
replacer = undefined;
- space = 0
}The overloads, type guard, and restructured function body are unnecessary for this fix and introduce maintenance overhead.
There was a problem hiding this comment.
Pull request overview
This PR updates EJSON.stringify to support additional call signatures (notably (value, options, space)), aligning its ergonomics more closely with JSON.stringify, and adds tests to validate supported overload combinations.
Changes:
- Added overloads and new runtime argument-disambiguation logic for
EJSON.stringify. - Introduced an internal type guard used to distinguish
optionsfromreplacer/space. - Added a new test suite covering many parameter signature combinations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/extended_json.ts |
Adds overloads and new argument parsing for EJSON.stringify via an internal options type guard. |
test/node/extended_json.test.ts |
Adds test coverage for supported EJSON.stringify parameter combinations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi @tadjik1 |
|
hi @chdanielmueller, thanks a lot for updating PR 👍 Just wanted to confirm that this work is not lost, and it's tracked. I will re-review changes as soon as possible. |
PavelSafronov
left a comment
There was a problem hiding this comment.
Changes look good, thanks for this!
Description
Summary of Changes
This pull request enhances the flexibility and robustness of the
EJSON.stringifyfunction by supporting multiple parameter signature combinations, similar toJSON.stringify.It introduces a new internal type guard to accurately distinguish between options and replacer parameters, and adds comprehensive tests to ensure correct behavior across all supported overloads.
It supports a previously advertised function signature which was not implemented.
EJSON.stringify(object, { relaxed: false }, 2)Notes for Reviewers
The tests should contain every previously possible and also previously advertised but not working combination of inputs.
The changes within the function have to do with the previous typescript signature which has been used but did not work. See
js-bson/test/bench/etc/generate_documents.ts
Line 27 in 0712bb1
Off Topic: Documented within NODE-7437
Release Highlight
EJSON.stringifynow correctly handles all parameter combinationsPreviously, calling
EJSON.stringify(value, options, space)silently discarded thespaceparameter, producing unformatted output. This has been fixed so all documented call signatures work as expected:Thanks to @chdanielmueller for working on this problem!
Double check the following
npm run check:lint)type(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript