[Fiber] fix ensure class components refs get re-assigned to emptyObject on mount

[trueadm]

Due to how Jest and other mocking libraries work in user-land, modules that are required might not be referentially equal at various stages of the application lifecycle. Previously, Stack would re-assign emptyObject upon mounting a class component, ensuring the reference to emptyObject remained consistent. This PR re-applies this change to class components in Fiber.

[sebmarkbage]

Why doesn't the same problem happen with Stack?

Regardless seems like we should be fixing this in jest since the runtime code doesn't this indirection.

Inlining doesn't help us switch to all ES modules and rollup.

[gaearon]

Stack assigns refs once again on the instance after constructor runs. So it overwrites the emptyObject referenced by isomorphic with emptyObject referenced by reconciler.

I guess we should do the same in mountClassInstance().

[trueadm]

That sounds like a better fix. Is that ideal in the long term though?

[gaearon]

In long term we'd get away from string refs anyway.

[sebmarkbage]

Seems like the action item is to fix the test environment.

[trueadm]

@sebmarkbage @gaearon changes have been made to the test suite and the other changes have been reverted. Thanks for the feedback.

[gaearon]

Hm, could you explain how this is related to automocking?
If you mean resetting modules, it's a different feature from automocking.
I don't think we have automocking enabled in React repo.

[trueadm]

This message was wrong, I've changed it. I didn't mean to state automocking.

[gaearon]

Why are we fixing the test environment rather than applying the same strategy as in Stack? Fixing the test might work for us, but I'm not sure it would work for consumers who also use Jest with resetModules.

[gaearon]

Even regardless of the test environment, the issue is likely to come up when you have different versions of React and ReactDOM, and thus potentially duplicated fbjs. We've been trying to reduce the potential breakage in such situations, and it would be nice not to add another case. Reassigning instance.refs in the renderer like we do in Stack seems like a straightforward solution for this.

Also, don't we need to reassign them for factory components anyway? I assume they support string refs in Stack although I haven't checked.

[trueadm]

@gaearon I don't think defining the classes at the top scope and using resetModules() is the right approach. It's better to have a guarantee that we're creating during beforeEach() remain consistent due to the side-effects from resetModules(). I do however agree that we should deal with these cases differently to avoid issues in the wild.

[trueadm]

@gaearon Ideally, ReactDOM would pull emptyObject from the React instance itself like Inferno does. The problem is that this won't currently work with our system as we require modules directly, rather than only using API endpoints. This would allow all modules that depend on React to access the same object, rather than potentially having many emptyObjects in existence.

[gaearon]

So, here's the case I'm concerned about:

  fit('wat', () => {
    function Comp() {
      return {
        render() {
          return <div ref="haha" />
        }
      }
    }

    var container = document.createElement('div');
    var inst = ReactDOM.render(<Comp />, container);
    expect(inst.refs.haha.tagName).toBe('DIV');
  })

This works in Stack because it reassigns instance.refs.
This crashes in Fiber because it doesn't.

If we fix Fiber to also reassign instance.refs, we'll also solve the problem this PR was created to solve. Two birds with one stone.

[gaearon]

(It wouldn't be a permanent solution but we don't depend on emptyObject reference equality anywhere else. If we start depending on it in more places, we can expose it from React just like we expose createElement from it, a la React.__emptyObject).

[trueadm]

@gaearon I agree with you. As a side note: the whole emptyObject optimization should ideally be used in as many places as we can (props, state etc) where nothing has been defined.

I'll update the PR to include a re-assignment for emptyObject. :)

[trueadm]

I've added a regression test for refs in factory components. @gaearon

seems like we needed to fix another bug