Improving API for unstable shouldUpdate and onUpdate

[bfiss]

Addressing issue #8 . Thanks @gaearon for the reviews.

This only exposes:

1. a ref, same that would be provided by React when using ReactDOM.render(...) and refs in general;
2. a file name where that ref is defined.

[wmertens]

Will this allow inspecting the ref to see what context names it's subscribed to? If not, that would be useful information to expose as well.

[bfiss]

@wmertens is that information available in this.context within a React instance? If so, then yes, it has that as well as props, state and any instance properties. Otherwise, it might be available in this.constructor, which should also be available for class instances. You can check it out by executing ReactDOM.render(...) on a node that is subscribed to context names, and seeing if what's returned contains what you are looking for.

[wmertens]

I am not sure, this is the approach I took before: https://github.com/gaearon/react-deep-force-update/pull/4/files#diff-1fdf421c05c1140f6d71444ea2b27638R25

Basically, if the contextName is a function on the class's contextTypes, that would be the correct test, but not sure if we can get at that information via ref?

[bfiss]

@wmertens, I see from your code that you were using internalInstance._instance.context[contextName], and ref is in fact internalInstance._instance (on stack-based React), so the use case is definitely still possible.

[gaearon]

Seems unfortunate to call it if it's not used. I guess this is the point where I ask you to change it back to check the function existence inline (and lazily calculate this if either function exists). Sorry!

[gaearon]

Let's rename this field to publicInstance. The instance doesn't really have a ref and it can be a bit confusing to overload this term.

[bfiss]

(y)

[gaearon]

Why are you reading filename from the child rather than from the component itself?

[bfiss]

From what I could see while playing around in Power Editor, if I'm looking for an instance of component Foo, defined in foo.js, It will be Foo's children who will have _debugSource.fileName === 'foo.js'. I couldn't test this with stack-based React since it seems it's not that easy to turn that on for Facebook code anymore, but for Fiber, it's working really well in practice.

[gaearon]

Maybe, I just don't understand why. This doesn't really make sense to me: a child is the child component, not the current component. Maybe you could give an example of component I could look at where children have filenames but parents don't?

[bfiss]

We talked about this offline. Seems like we can use .displayName, which isn't always available, or use this heuristic, or something else I haven't thought of. Here's what's usually available:

[bfiss]

Sounds good, will update soon.

[bfiss]

This removes support for shouldUpdate and onUpdate for stack-based React. We should make sure to address #10.

[gregberge]

@bfiss you should add test on Fiber if possible

[bfiss]

Talked with @gaearon and we decided to upgrade React to 16. This will automatically get the tests on Fiber.

[bfiss]

There's a new warning, but I guess it should be okay to ignore:

Warning: React depends on requestAnimationFrame. Make sure that you load a polyfill in older browsers. http://fb.me/react-polyfills

[chabou]

Thank you so much for your work 🙏
I really need to have this PR merged.
Any ETA?

[gregberge]

I didn't follow it, @bfiss @gaearon any update?

[bfiss]

@neoziro @chabou @gaearon this is being used extensively within Facebook, which is on Fiber. This was just waiting for the tests to actually run on Fiber, it should be good to go since the tests are running on Fiber now.

[gregberge]

@gaearon @bfiss even I think we should document shouldUpdateClosestClassInstance and onUpdateClassInstance in the readme. Also it is a breaking change. We should release it under a v3 right?

[gaearon]

We'll need to avoid reading the tag field. This means both changing the shouldUpdate callback to not take it, and changing the class check to something like

  const hasInstance =
    node.stateNode !== null &&
    typeof node.type === 'function';

(I already upstreamed this change on www)