Skip to content
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

Avoid needless hasOwnProperty check in deepFreeze. #3545

Merged
merged 2 commits into from
Jun 5, 2018

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Jun 5, 2018

The "own" in getOwnPropertyNames ensures that all the names would pass the hasOwnProperty test.

Regarding this comment by @jamesreggio, I would much rather patch apollo-utilities than revert the use of Object.create(null), since empty prototype-free objects never accidentally appear to have properties inherited from Object.prototype (a correctness concern), and lookups of missing properties are faster since there's no prototype chain. Far from being deficient, Object.create(null) objects are what empty objects should always have been in JavaScript.

@benjamn benjamn self-assigned this Jun 5, 2018
The "own" in getOwnPropertyNames ensures that all the names would pass the
hasOwnProperty test.

#3300 (comment)

Regarding this comment by @jamesreggio, I would much rather patch
apollo-utilities than revert the use of Object.create(null), since empty
prototype-free objects never accidentally appear to have properties
inherited from Object.prototype (a correctness concern), and lookups of
missing properties are faster since there's no prototype chain. Far from
being deficient, Object.create(null) objects are what empty objects should
always have been in JavaScript.
@benjamn benjamn force-pushed the fix-apollo-utilities-hasOwnProperty-call branch from 08fe5c3 to fb5d021 Compare June 5, 2018 18:35
@apollo-cla
Copy link

apollo-cla commented Jun 5, 2018

Warnings
⚠️

There are library changes, but not tests. That's OK as long as you're refactoring existing code

Generated by 🚫 dangerJS

@benjamn benjamn merged commit fb06b42 into master Jun 5, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants