Fix ReactFiberReconciler annotation to include `PI` #8751

iamdustan · 2017-01-11T19:49:52Z

Fixing up @sebmarkbage comments at #8628 (comment)

sebmarkbage · 2017-01-11T20:05:22Z

src/renderers/testing/ReactTestRendererFiber.js

@@ -189,7 +189,7 @@ var TestRenderer = ReactFiberReconciler({

  useSyncScheduling: true,

-  getPublicInstance(inst) {
+  getPublicInstance(inst : Instance | TextInstance) {


It would be nice to have the return value here too to guarantee that the inference is correct. Otherwise, if it gets passed somewhere else, it might be inferred to be something very simple (like an empty object) and passes everywhere.

how do I say the return value is Instance | TextInstance | createNodeMockReturnValue?

Ah, I see. Just put getPublicInstance(inst : Instance | TextInstance) : * then for "inferred". That way if createNodeMock gets a proper type in the future, it'll get caught.

sebmarkbage · 2017-01-11T20:09:21Z

src/renderers/shared/fiber/ReactFiberReconciler.js

@@ -170,7 +170,7 @@ module.exports = function<T, P, I, TI, PI, C, CX>(config : HostConfig<T, P, I, T

    deferredUpdates,

-    getPublicRootInstance(container : OpaqueNode) : (ReactComponent<any, any, any> | I | TI | null) {
+    getPublicRootInstance(container : OpaqueNode) : (ReactComponent<any, any, any> | I | TI | PI | null) {


The purpose of this method is the same as for refs except for the return value on the very top level. var inst = ReactDOM.render(...)

It should never be possible to get a handle on an instance of I or TI since those are internal instances to the renderer. The point of getPublicInstance is to ensure that.

Therefore this should only return ReactComponent<any, any, any> | PI | null.

To ensure that you must call getPublicInstance on the stateNode for the appropriate tag.

sebmarkbage

See comments.

iamdustan · 2017-01-11T20:31:04Z

Updated @sebmarkbage. Thanks!

sebmarkbage · 2017-01-11T20:51:21Z

src/renderers/shared/fiber/ReactFiberReconciler.js

@@ -170,7 +170,7 @@ module.exports = function<T, P, I, TI, PI, C, CX>(config : HostConfig<T, P, I, T

    deferredUpdates,

-    getPublicRootInstance(container : OpaqueNode) : (ReactComponent<any, any, any> | I | TI | null) {
+    getPublicRootInstance(container : OpaqueNode) : (ReactComponent<any, any, any> | PI | null) {


This function still needs to pass stateNode to getPublicInstance for the relevant tags.

how could I reproduce this scenario? None of the current tests seem to have an issue. I attempted to reproduce it with a root ref and the return value of render with a few component types.

describe('root level refs', () => { it('attaches and detaches root refs in stack', () => { assertForRenderer(require('ReactDOM')); }); it('attaches and detaches root refs in fiber', () => { assertForRenderer(require('ReactDOMFiber')); }); const assertForRenderer = (DOM) => { spyOn(console, 'error'); var ref = jest.fn(); var container = document.createElement('div'); var result = DOM.render(<div ref={ref} />, container); expect(ref).toHaveBeenCalledTimes(1); expect(ref.mock.calls[0][0]).toBeInstanceOf(HTMLDivElement); expect(result).toBe(ref.mock.calls[0][0]); DOM.unmountComponentAtNode(container); expect(ref).toHaveBeenCalledTimes(2); expect(ref.mock.calls[1][0]).toBe(null); class Comp extends React.Component { render() { return <div>Comp</div>; } } ref = jest.fn(); result = DOM.render(<Comp ref={ref}/>, container); expect(ref).toHaveBeenCalledTimes(1); expect(ref.mock.calls[0][0]).toBeInstanceOf(Comp); expect(result).toBe(ref.mock.calls[0][0]); DOM.unmountComponentAtNode(container); expect(ref).toHaveBeenCalledTimes(2); expect(ref.mock.calls[1][0]).toBe(null); }; });

I don’t think that getPublicRootInstance will ever call getPublicInstance. They both seem to be added for backwards compatibility with different features and operate independently.

well, what should this be:

var result = ReactDOMFiber.render(5, container); // result? '5' or fiber?

Public API must never expose fibers.

Since this exists for Stack compat I would return nothing. Stack doesn't allow rendering top-level text nodes so nothing would break if we didn't implement this.

If we did want to implement it (I don't see why) then it would make sense to return the TextInstance (text node), similar to how ReactDOM.render(<div />) returns the Instance (div).

I think this is “current” behavior. Even if I remove the getPublicInstance added from the ref, the result in that example is not '5' as I expected. I think, though, that that is a different issue..

The more I think about this the less I’m convinced that this should be changed from I | TI to PI. PI was only added for getPublicInstance which is only called via attachRef. The naming may be unfortunate since getHostPublicInstance and getPublicInstance aren’t related (internally).

Fwiw, I’m not attempting or desiring to implement anything new here, just trying to understand what the behavior was before #8628.

var instA; var instB = MyRenderer.render(<div ref={n => instA = n} />); expect(instA).toBe(instB);

Is that always true with the test renderer?

Ah yeah. That’d do it. Test case and possible solution implemented in 9a9a4db.

facebook#8751 (comment)

sebmarkbage · 2017-01-18T17:09:14Z

src/renderers/shared/fiber/ReactFiberReconciler.js

      const root : FiberRoot = (container.stateNode : any);
      const containerFiber = root.current;
      if (!containerFiber.child) {
        return null;
      }
-      return containerFiber.child.stateNode;
+      return getPublicInstance(containerFiber.child.stateNode);


This still suffers from this problem: https://github.com/facebook/react/pull/8628/files/9a9276b3fbcec4cfb6d5c69e891c5feafe0195f2#r95653360 Which remains a problem in commitWork.

You need to check whether containerFiber.child.tag === HostComponent before passing it to getPublicInstance.

Otherwise, in the case of render(<Foo />) you will pass an instance of Foo to getPublicInstance which is neither an I nor TI so the function doesn't know what to do with it.

This will become more clear after #8545 because then the stateNode will have a proper type and Flow would've disallowed passing it to getPublicInstance.

https://github.com/facebook/react/pull/8751/files#diff-7002876230f7effe9aa22e292f6a52c6R463 handles it in CommitWork. Added the switch for this method as well to do the same for getRootPublicInstance. Some tests have regressed that I’ll need to look into why.

Shouldn’t render(<Foo />) pass the I | TI back for backwards compatibility?

facebook#8628 (comment)

facebook#8751 (comment)

gaearon · 2017-05-30T15:14:37Z

src/renderers/shared/shared/__tests__/refs-test.js

+  });
+
+  it('attaches and detaches root refs in fiber', () => {
+    assertForRenderer('ReactDOMFiber');


Generally we try to avoid code that is as dynamic as this. It makes it harder to trace what's going on.

gaearon · 2017-05-30T15:35:04Z

I think this might be ready for another look.

sebmarkbage · 2017-05-30T17:18:47Z

src/renderers/shared/fiber/ReactFiberReconciler.js

+      switch (containerFiber.child.tag) {
+        case HostComponent:
+          return getPublicInstance(containerFiber.child.stateNode);
+        default:


This case should match on ClassComponent only. It won't type check if we use proper disjoint unions on Fibers.

That type error would be correct since this can be other types such as fragments, coroutines etc. Those should do something reasonable like throw, drill down (like findDOMNode does) or return null.

It won't type check if we use proper disjoint unions on Fibers.

Is that PR any closer to landing by chance?

It'll have to be a massive rebase. We could try it again though. There are some new features in Flow that can make that palatable now.

sebmarkbage

Accepting because it is no more buggy than what is already there but I left a comment.

gaearon · 2017-05-30T19:16:11Z

OK. I merged and added followup as a child of existing todo in #7925.

facebook-github-bot added CLA Signed GH Review: review-needed labels Jan 11, 2017

gaearon requested a review from sebmarkbage January 11, 2017 20:03

sebmarkbage reviewed Jan 11, 2017

View reviewed changes

sebmarkbage requested changes Jan 11, 2017

View reviewed changes

iamdustan force-pushed the fiber-test-renderer-refinements branch from 9d42ea0 to 41bdb14 Compare January 11, 2017 20:28

sebmarkbage reviewed Jan 11, 2017

View reviewed changes

iamdustan added a commit to iamdustan/react that referenced this pull request Jan 13, 2017

attempting to trigger the issue @sebmarkbage described

e6f2186

facebook#8751 (comment)

iamdustan force-pushed the fiber-test-renderer-refinements branch from e6f2186 to 2a705c6 Compare January 13, 2017 12:57

iamdustan added a commit to iamdustan/react that referenced this pull request Jan 13, 2017

attempting to trigger the issue @sebmarkbage described

2a705c6

facebook#8751 (comment)

iamdustan force-pushed the fiber-test-renderer-refinements branch from 2a705c6 to f78245f Compare January 17, 2017 01:05

iamdustan added a commit to iamdustan/react that referenced this pull request Jan 17, 2017

attempting to trigger the issue @sebmarkbage described

693eaed

facebook#8751 (comment)

iamdustan force-pushed the fiber-test-renderer-refinements branch from f78245f to 9a9a4db Compare January 17, 2017 01:06

iamdustan added a commit to iamdustan/react that referenced this pull request Jan 17, 2017

attempting to trigger the issue @sebmarkbage described

1f04089

facebook#8751 (comment)

sebmarkbage reviewed Jan 18, 2017

View reviewed changes

iamdustan added 3 commits January 21, 2017 13:32

Fix ReactFiberReconciler annotation to include PI

c4bd356

facebook#8628 (comment)

Make getPublicInstance type safe

2e942c7

attempting to trigger the issue @sebmarkbage described

662844c

facebook#8751 (comment)

iamdustan force-pushed the fiber-test-renderer-refinements branch from 9a9a4db to 5156c65 Compare January 21, 2017 18:35

iamdustan added 2 commits January 21, 2017 13:36

Unify renderer-specific getPublicInstance with getRootInstance

fc91ebb

Switch on fiber type HostComponent for getPublicRootInstance

5156c65

sebmarkbage removed the GH Review: review-needed label Mar 1, 2017

Merge branch 'master' into fiber-test-renderer-refinements

a61637a

gaearon reviewed May 30, 2017

View reviewed changes

gaearon added 2 commits May 30, 2017 16:25

Fix test that was too dynamic (and failing)

e30fd4c

Use PI in reconciler public API

303eeff

Prettier

7018be8

sebmarkbage reviewed May 30, 2017

View reviewed changes

sebmarkbage approved these changes May 30, 2017

View reviewed changes

sebmarkbage mentioned this pull request May 30, 2017

[Fiber] Umbrella for remaining features / bugs #7925

Closed

gaearon merged commit 80bdebc into facebook:master May 30, 2017

iamdustan deleted the fiber-test-renderer-refinements branch June 1, 2017 19:25

Fix ReactFiberReconciler annotation to include PI #8751

Fix ReactFiberReconciler annotation to include PI #8751

Uh oh!

Conversation

iamdustan commented Jan 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebmarkbage left a comment

Choose a reason for hiding this comment

Uh oh!

iamdustan commented Jan 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon commented May 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebmarkbage left a comment

Choose a reason for hiding this comment

Uh oh!

gaearon commented May 30, 2017

Uh oh!

Uh oh!

Fix ReactFiberReconciler annotation to include `PI` #8751

Fix ReactFiberReconciler annotation to include `PI` #8751