Inline fbjs/lib/emptyObject #13055
Inline fbjs/lib/emptyObject #13055
Conversation
| @@ -157,7 +156,17 @@ function coerceRef( | |||
| return current.ref; | |||
| } | |||
| const ref = function(value) { | |||
| const refs = inst.refs === emptyObject ? (inst.refs = {}) : inst.refs; | |||
There was a problem hiding this comment.
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 duplicate fbjs), 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. But Object.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.
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.
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.
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.
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.
Pushed an alternative fix
| @@ -28,14 +27,19 @@ if (__DEV__) { | |||
| warnedAboutMissingGetChildContext = {}; | |||
| } | |||
|
|
|||
| export const emptyContextObject = {}; | |||
There was a problem hiding this comment.
This is exported because there are subtle assumptions around its referential equality in a few places throughout the Fiber codebase. It's used a sentinel value. This makes it explicit for this particular use case.
| // This is needed for some tests that rely on string refs | ||
| // but reset modules between loading different renderers. | ||
| const obj = require.requireActual('fbjs/lib/emptyObject'); | ||
| jest.mock('fbjs/lib/emptyObject', () => obj); |
There was a problem hiding this comment.
We can now drop this.
|
ReactDOM: size: 0.0%, gzip: 0.0% Details of bundled changes.Comparing: ae14317...d385fe3 react-dom
react-art
react-test-renderer
react-reconciler
react-native-renderer
Generated by 🚫 dangerJS |
|
I think we rely on reference equality for empty function too in some cases. What’s the goal of this? Why not just keep a shared instance but inside the React bundle? |
| const refs = inst.refs === emptyObject ? (inst.refs = {}) : inst.refs; | ||
| let refs; | ||
| // This flag tells us if we need to initialize `this.refs`. | ||
| if (inst.__reactInternalHasLegacyRefs) { |
There was a problem hiding this comment.
Small not-even-worth-mentioning nit: since this isn't always a boolean, might be better to explicitly compare to undefined instead, like we do for other optional types.
|
|
||
| import {TYPES, EVENT_TYPES, childrenAsString} from './ReactARTInternals'; | ||
|
|
||
| const pooledTransform = new Transform(); | ||
|
|
||
| const UPDATE_SIGNAL = {}; | ||
| const emptyObject = {}; |
There was a problem hiding this comment.
Why go from a descript explicit identity to a non-descript, easy to accidentally share object?
There was a problem hiding this comment.
I was on the fence about this. Even if it's shared, does it matter if it's frozen in DEV? Seems like you'd quickly catch the mistake. It's also used under this name for context. Do you want to have a separate EMPTY_CONTEXT too? I can revert that part.
There was a problem hiding this comment.
Pushed 1f0c930 which makes explicit naming consistent across renderers
Don't think so. I've already removed empty function in #13054. It wasn't used in comparisons.
I started doing that but then realized I don't see the point. It's just a few places. I also dislike that having it encourages implicit assumptions about reference equality which are later hard to debug. |
|
A lot of the legit reasons to use an object is as a tag that cannot be duplicated. Kind of like Symbol. I don’t necessarily see reference equality as a bad thing. It is only bad if two objects that are two conceptual different signals have shared identity. |
I agree, but that's exactly what happened. "Empty refs" and "empty context" and "empty host context" and "update signal" are all different things. I found it difficult to understand where it was important in the code and where it wasn't. So that's the motivation for not sharing them. The non-duplicatedness doesn't survive package boundaries without weird artifacts in edge cases. So I was trying to make it more local too. |
|
Some other alternatives for refs:
|
Each renderer would have its own local LegacyRefsObject function. While in general we don't want `instanceof`, here it lets us do a simple check: did *we* create the refs object? Then we can mutate it. If the check didn't pass, either we're attaching ref for the first time (so we know to use the constructor), or (unlikely) we're attaching a ref to a component owned by another renderer. In this case, to avoid "losing" refs, we assign them onto the new object. Even in that case it shouldn't "hop" between renderers anymore.
|
Okay, so I pushed an alternative in 2e006cc which doesn't rely on cross-package object identity working, and doesn't break hidden classes. It does use |
|
I see the problem you’re trying to fix now. I was under the impression that we set refs in the renderer and not the isomorphic package. For everything else we set it up in both. Eg updater can be different until after the constructor. So what if we do the same here and set it to the renderer’s instance? That way it is always the same. |
| const refs = inst.refs === emptyObject ? (inst.refs = {}) : inst.refs; | ||
| let refs; | ||
| // This flag tells us if we need to initialize `this.refs`. | ||
| if (inst.__reactInternalHasLegacyRefs === undefined) { |
There was a problem hiding this comment.
Checking for a nonexistent property is one of the most expensive things you can do if it is not cached. Because it has traverse the prototype chain.
| @@ -157,7 +156,17 @@ function coerceRef( | |||
| return current.ref; | |||
| } | |||
| const ref = function(value) { | |||
| const refs = inst.refs === emptyObject ? (inst.refs = {}) : inst.refs; | |||
There was a problem hiding this comment.
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?
We set in both. Normally checking for the renderer-owned I think it's insufficient if the component is created by renderer Like ReactDOM.render(<Parent />, root) // parent.refs = emptyObjectFromReactDOM
class Parent {
render() {
var el = <Child ref="child" /> // <--- Parent is owner
return <Indirection>{el}</Indirection>
}
}
class Indirection {
componentDidMount() {
AnotherReactDOMCopy.render(this.props.children, root2)
// element._owner.refs !== emptyObjectFromAnotherReactDOMCopy
// so it will think it's safe to assign to them, but they're frozen
}
}In other words, a ref owner isn't necessarily created by the same renderer. |
|
Interesting. If that is possible it is only because they share the current owner. In which case they have a shared isomorphic package. The reason I’m pushing on this is since we have plans on relying even more on a shared isomorphic package and that I generally think of the OCaml/Rust style of variants to be the appropriate way to handle conditional states. So it’s interesting to explore beyond this change. I don’t like exposing objects with prototype chains because people inspect them and it gets different hidden classes than other objects. Typically this gets an empty object with expandos. |
|
Wouldn't it switch to dictionary mode anyway as soon as we |
|
I added a regression test for the case I explained above (which is easy to break with alternative solutions), and pushed a different approach. I'm relying on |
It's not currently possible to resetModules() between several renderers without also resetting the `React` module. However, that leads to losing the referential identity of the empty ref object, and thus subsequent checks in the renderers for whether it is pooled fail (and cause assignments to a frozen object). This has always been the case, but we used to work around it by shimming fbjs/lib/emptyObject in tests and preserving its referential identity. This won't work anymore because we've inlined it. And preserving referential identity of React itself wouldn't be great because it could be confusing during testing (although we might want to revisit this in the future by moving its stateful parts into a separate package). For now, I'm removing string ref usage from this test because only this is the only place in our tests where we hit this problem, and it's only related to string refs, and not just ref mechanism in general.
|
The last commit changes the ReactART test because it hits an unfortunate edge case (string refs + resetModules) that breaks the current setup. To be clear it was broken before too, but shimming A longer term fix might be to extract all shared state (including current owner etc) into yet another package as we discussed at some point, and then always preserving it across module resets, and locking its version. |
|
I'm going to land this. Looks like sharing refs was the only concern and Seb suggested putting it on the React object, I did it through |
* Inline fbjs/lib/emptyObject * Explicit naming * Compare to undefined * Another approach for detecting whether we can mutate Each renderer would have its own local LegacyRefsObject function. While in general we don't want `instanceof`, here it lets us do a simple check: did *we* create the refs object? Then we can mutate it. If the check didn't pass, either we're attaching ref for the first time (so we know to use the constructor), or (unlikely) we're attaching a ref to a component owned by another renderer. In this case, to avoid "losing" refs, we assign them onto the new object. Even in that case it shouldn't "hop" between renderers anymore. * Clearer naming * Add test case for strings refs across renderers * Use a shared empty object for refs by reading it from React * Remove string refs from ReactART test It's not currently possible to resetModules() between several renderers without also resetting the `React` module. However, that leads to losing the referential identity of the empty ref object, and thus subsequent checks in the renderers for whether it is pooled fail (and cause assignments to a frozen object). This has always been the case, but we used to work around it by shimming fbjs/lib/emptyObject in tests and preserving its referential identity. This won't work anymore because we've inlined it. And preserving referential identity of React itself wouldn't be great because it could be confusing during testing (although we might want to revisit this in the future by moving its stateful parts into a separate package). For now, I'm removing string ref usage from this test because only this is the only place in our tests where we hit this problem, and it's only related to string refs, and not just ref mechanism in general. * Simplify the condition
Continuing #13046 and #13054.
This one was a bit more challenging, but it was nice to uncover the current assumptions. (Which tended to break with module resetting, for example.)
In most places I just inlined the code to create a DEV-frozen object. But a few places relied on reference equality semantics. I will add inline comments to describe what I did to fix those.