fix(NODE-7436): EJSON.stringify type signature by chdanielmueller · Pull Request #870 · mongodb/js-bson

chdanielmueller · 2026-02-10T10:28:03Z

Description

Summary of Changes

This pull request enhances the flexibility and robustness of the EJSON.stringify function by supporting multiple parameter signature combinations, similar to JSON.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

bson.EJSON.stringify(singleField, { relaxed: false }, 2)

Off Topic: Documented within NODE-7437

I noticed that the array replacement signature for JSON.stringify replaces the serialized keys.
const testDoc = {
  objectId: ObjectId.createFromHexString('111111111111111111111111')
};

const result = EJSON.stringify(testDoc, ['objectId', '$oid']) // {"objectId":{"$oid":"111111111111111111111111"}}
const result2 = EJSON.stringify(testDoc, ['objectId']) // {"objectId":{}}
Maybe if the replacer is an Array it should add the data types to the array.

Release Highlight

Release notes highlight

Double check the following

Lint is passing (npm run check:lint)
Self-review completed using the steps outlined here
PR title follows the correct format: type(NODE-xxxx)[!]: description
- Example: feat(NODE-1234)!: rewriting everything in coffeescript
Changes are covered by tests
New TODOs have a related JIRA ticket

PavelSafronov · 2026-02-10T18:38:45Z

@chdanielmueller , thanks so much for your contribution! The team is going to prioritize this PR and NODE-7436 at our next triage session.

tadjik1

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.

tadjik1 · 2026-02-27T09:15:34Z

src/extended_json.ts

+  }
+
+  // Check that there are no invalid properties (only known EJSON serialize options)
+  const validKeys = ['legacy', 'relaxed', 'ignoreUndefined'];


This creates a maintenance footgun with to compiler warning to catch if EJSONSerializeOptions gains a new property, or if detection silently breaks. In this case the new property would be treated as a replacer and passed into JSON.stringify.

Removed by d3047c3

tadjik1 · 2026-02-27T09:18:55Z

src/extended_json.ts

+  if ('legacy' in value && typeof value.legacy !== 'boolean') {
+    return false;
+  }
+  if ('relaxed' in value && typeof value.relaxed !== 'boolean') {
+    return false;
+  }
+  if ('ignoreUndefined' in value && typeof value.ignoreUndefined !== 'boolean') {
+    return false;
+  }


The strict key check causes silent misbehavior on typos. If something like {relaxd: false} will be passed - this guard returns false and the object will silently be forwarded to JSON.stringify as a replacer - producing wrong output, the existing typeof replacer === 'object' && !Array.isArray(replacer) would correctly treat it as options. This is more forgiving and safer.

That's a great catch. I did not think of that.

I simplified the type guard to the old implementation.
I added a new test to validate the behavior in the presence of typos and to ensure this decision is explicitly captured.
see d3047c3

Type guard removed completely in ee14858

test/node/extended_json.test.ts

tadjik1 · 2026-02-27T09:27:00Z

src/extended_json.ts

+  if (isEJSONSerializeOptions(replacerOrOptions)) {
+    options = replacerOrOptions;
    replacer = undefined;
-    space = 0;


I think this is the original bug and removing just this line (avoid unconditional discarding the space value( would make everything work as expected.

Whilst I think, that my approach would have improved readability for the next developer I see your concern.
I did revert the implementation to the previous implementation but kept the new variable names as an indicator that it can be either or.
ee14858

tadjik1 · 2026-02-27T09:30:20Z

src/extended_json.ts

+ * console.log(EJSON.stringify(doc, { relaxed: false }, 2));
 * ```
 */
 function stringify(


These overloads add type-level documentation of supported call patterns, but they also add maintenance hurdle. Since the runtime checks are needed regardless, and the existing signature with the union type already communicates that options can go in the second position - this doesn't provide enough value to justify the complexity.

I appreciate the concern about complexity. From my perspective, the overloads provide meaningful value: they catch invalid parameter combinations (e.g., passing space before options when replacer is omitted) at compile-time and make IDE autocomplete show three clear patterns instead of a confusing union type.
In my opinion the ~15 lines of declarations add negligible maintenance cost while significantly improving type safety and DX.

Copilot

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 options from replacer/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.

src/extended_json.ts

test/node/extended_json.test.ts

chdanielmueller · 2026-03-03T10:59:34Z

Hi @tadjik1
Thank you for the feedback and the review. I did address all your comments with the new changes with one exception: #870 (comment)
Giving it back to you for another review 😄

tadjik1 · 2026-03-19T11:49:47Z

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.

chdanielmueller added 4 commits February 10, 2026 11:05

Tests to proof NODE-7436

d46b85f

Improve signature for typescript and add missing functionality

6d5b35b

Replace with more complete tests

e2dd8fd

Linting

cc24320

chdanielmueller requested a review from a team as a code owner February 10, 2026 10:28

PavelSafronov added tracked-in-jira There is a ticket in Mongo's Jira instance tracking this issue/PR External Submission labels Feb 10, 2026

tadjik1 requested changes Feb 27, 2026

View reviewed changes

tadjik1 self-assigned this Feb 27, 2026

tadjik1 added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Feb 27, 2026

Simplify type guard to handle typos

d3047c3

Copilot AI review requested due to automatic review settings March 3, 2026 10:07

Copilot started reviewing on behalf of chdanielmueller March 3, 2026 10:07 View session

Copilot AI reviewed Mar 3, 2026

View reviewed changes

src/extended_json.ts Outdated Show resolved Hide resolved

test/node/extended_json.test.ts Show resolved Hide resolved

chdanielmueller added 2 commits March 3, 2026 11:47

Remove type guard and simplify implementation

ee14858

Format

a5a89d5

chdanielmueller requested a review from tadjik1 March 3, 2026 10:59

Conversation

chdanielmueller commented Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Summary of Changes

Notes for Reviewers

Release Highlight

Release notes highlight

Double check the following

Uh oh!

PavelSafronov commented Feb 10, 2026

Uh oh!

tadjik1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chdanielmueller Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

chdanielmueller commented Mar 3, 2026

Uh oh!

tadjik1 commented Mar 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chdanielmueller commented Feb 10, 2026 •

edited

Loading

chdanielmueller Mar 3, 2026 •

edited

Loading