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
Inline fbjs/lib/emptyObject #13055
Merged
Merged
Inline fbjs/lib/emptyObject #13055
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
f1ddfdc
Inline fbjs/lib/emptyObject
gaearon 1f0c930
Explicit naming
gaearon 39b9b9d
Compare to undefined
gaearon 2e006cc
Another approach for detecting whether we can mutate
gaearon 579e37c
Clearer naming
gaearon 4c7a251
Add test case for strings refs across renderers
gaearon 6429ab7
Use a shared empty object for refs by reading it from React
gaearon d76d092
Remove string refs from ReactART test
gaearon d385fe3
Simplify the condition
gaearon File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part was a bit shady.
By default we set
inst.refs
to a frozen, shared object (in fact, we do this in two places: Component constructor, and later in the renderer). Whenever we want to mutate a ref for the first time, we check whether it's still "the same" pooled object, and if it is, we swap it out with the mutable one.In some obscure cases where
emptyObject
identity changes over time (e.g. due to a module reset in Jest, or due to a duplicatefbjs
), this wouldn't work and cause very confusing errors.I've changed this to use an explicit flag instead. It is only set and read in this module. By default it's not set.
When the flag is not set, the first attempt to set a legacy ref will know that we need to create a new mutable refs object. Then we set the flag. Next time we attach a ref, the flag tells us it's safe to mutate now.
I could have put the flag on the mutable refs object instead. But that could break code like
Object.keys(this.refs).map(...)
. I could make it non-enumerable. ButObject.defineProperty
is kinda slow. I figured an instance property is a fair game because we already use a bunch of those for legacy context.This only affects classes using string refs. There is no cost for classes that don't use them. Even for those that do, it's a single boolean field.
This lets us get rid of the dependency on strict equality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You’re messing with the hidden classes here so there can still be a cost.
I’m not sure this is worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're already messing with them for legacy context. The only kind of components I'm worried about regressing (Relay) has both legacy context and legacy refs. This gives an incentive to migrate off both without regressing the current situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess context is more localized in practice than legacy refs. Maybe not worth risking.
I can keep it on the React object but I think this will require some mocking of React itself in tests to ensure the object is preserved throughout
resetModules
. It was a bit easier for a separate module.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t see how the problems you mentioned trying to fix are still problems after this change even without this.
Even before I’m not sure how that could happen before but since it is now inline here, how would this happen? Somehow try to reuse the instance in the same renderer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed an alternative fix