Skip to content

Conversation

@yungsters
Copy link
Contributor

Summary:
Currently, ReactCompositeComponent mutates values returned by getDefaultProps, getInitialState, and getChildContext. This is bad because the objects may, for example, be cached and re-used across instances of a React component.

This changes ReactCompositeComponent to instead create a new object. In return for allocating a new object, I've replaced mapObject with forEachObject so that we are no longer allocating an unused object.

Fair trade, IMO.

Test Plan:
Ran unit tests successfully:

npm run jest

@sophiebits
Copy link
Collaborator

Perhaps worth calling it mergeIntoWithNoDuplicateKeys or something to emphasize that it's like mergeInto?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... shouldn't this be !(key in one)? Keys with the value of undefined have feelings, too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be hasOwnProperty, I guess.

@yungsters
Copy link
Contributor Author

Revised without forEachObject!

@syranide
Copy link
Contributor

Perhaps it's too performance sensitive to justify it, but wouldn't it make sense to use Object.assign instead but just "manually" assert that there are no duplicate keys... perhaps even change the invariant to a warning and only do it for DEV?

@yungsters
Copy link
Contributor Author

See #2464 (which coincidentally has the same numbers as this pull request, but shuffled).

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.

4 participants