Skip to content

fix(node/assert): throw on deepStrictEqual({}, Object.create(null)) #29428

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

edilson258
Copy link

assert.deepStrictEqual now differentiates {} from Object.create(null)

This PR updates the object comparison logic to throw when comparing a plain object ({}) with an object created via Object.create(null), matching Node.js behavior.

Closes #27565

@edilson258
Copy link
Author

edilson258 commented May 24, 2025

There are some tests failing because they expect the old behavior where deepStrictEqual({}, Object.create(null)) is true, like:

t.assert.deepStrictEqual(
db.prepare('BEGIN').run(),
{ changes: 0, lastInsertRowid: 0 },
);
t.assert.deepStrictEqual(
db.prepare('INSERT INTO data (key) VALUES (100)').run(),
{ changes: 1, lastInsertRowid: 100 },
);
t.assert.deepStrictEqual(
db.prepare('COMMIT').run(),
{ changes: 1, lastInsertRowid: 100 },
);

All the above assertions now fails because db.prepare('BEGIN').run() returns an object whose prototype is null __proto__: null, whereas the expected object { changes: 0, lastInsertRowid: 0 } has the default Object.prototype. Since assert.deepStrictEqual NOW checks both property values and the prototype chain, the difference in prototypes causes the assertion to fail.

I think that setting __proto__: null in the expected object is the best solution because it ensures that the object has no prototype and is an approach that is already being used in similar cases, like:

t.assert.deepStrictEqual(
db.prepare('SELECT * FROM data').all(),
[{ __proto__: null, key: 100 }],
);

There are lots of assertions in the code base that rely on that old behavior, this patch will touch lots of files in the source tree.

Is that proposed change acceptable?

@kt3k
@bartlomieju

@kt3k
Copy link
Member

kt3k commented May 26, 2025

@edilson258 Thanks for the PR. The maintainers will work on the fixes for the existing test cases (they should be passing as is). Please wait for a moment.

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.

ext/node: assert.deepStrictEqual({}, Object.create(null)) should throw
2 participants