Skip to content

Use Object.create(null) over {} to avoid prototype issues #4634

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.

Sign up for GitHub

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
benjie wants to merge 1 commit into
base: 17.x.x
Choose a base branch
from
+4 −4
Open

Use Object.create(null) over {} to avoid prototype issues #4634

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/execution/values.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ export function getArgumentValues(
fragmentVariableValues?: Maybe<FragmentVariableValues>,
hideSuggestions?: Maybe<boolean>,
): { [argument: string]: unknown } {
const coercedValues: { [argument: string]: unknown } = {};
const coercedValues: { [argument: string]: unknown } = Object.create(null);

const argumentNodes = node.arguments ?? [];
const argNodeMap = new Map(argumentNodes.map((arg) => [arg.name.value, arg]));
Expand All @@ -224,7 +224,7 @@ export function getArgumentValues(
hideSuggestions,
);
}
return coercedValues;
return { ...coercedValues };
Copy link
Copy Markdown
Contributor

@yaacovCR yaacovCR Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this strictly necessary?

Copy link
Copy Markdown
Member Author

@benjie benjie Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without it, it's a breaking change (assuming this is reachable from user code) since you'd be returning a null prototype object rather than a prototypeful object. Users who did console.log(`Blah blah ${obj} blah blah`) would have their code change from logging Blah blah [object Object] blah blah to throwing an error.

> const a = {}
undefined
> const b = Object.create(null)
undefined
> String(a)
'[object Object]'
> String(b)
Uncaught TypeError: Cannot convert object to primitive value
    at String (<anonymous>)

The reason I didn't merge this myself is because ideally we'd just switch to null prototype objects and get rid of the problem entirely. But I didn't want to introduce a breaking change, hence including this careful back-compat.

Personally I vote to remove this.

}

// eslint-disable-next-line max-params
Expand Down
4 changes: 2 additions & 2 deletions src/utilities/coerceInputValue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ export function coerceInputValue(
return; // Invalid: intentionally return no value.
}

const coercedValue: any = {};
const coercedValue: any = Object.create(null);
const fieldDefs = type.getFields();
const hasUndefinedField = Object.keys(inputValue).some(
(name) => !Object.hasOwn(fieldDefs, name),
Expand Down Expand Up @@ -109,7 +109,7 @@ export function coerceInputValue(
}
}

return coercedValue;
return { ...coercedValue };
Copy link
Copy Markdown
Contributor

@yaacovCR yaacovCR Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here?

}

const leafType = assertLeafType(type);
Expand Down
Loading