Fix use of stale props in Fabric events

[rubennorte]

Summary

We had to revert the last React sync to React Native because we saw issues with Responder events using stale event handlers instead of recent versions.

I reviewed the merged PRs and realized the problem was in the refactor I did in #26321. In that PR, we moved currentProps from canonical, which is a singleton referenced by all versions of the same fiber, to the fiber itself. This is causing the staleness we observed in events.

This PR does a partial revert of the refactor in #26321, bringing back the canonical object but moving publicInstance to one of its fields, instead of being the canonical object itself.

How did you test this change?

Existing unit tests continue working (I didn't manage to get a repro using the test renderer).
I manually tested this change in Meta infra and saw the problem was fixed.

[gaearon]

This was originally written in 051637d, so you can see how we test this kind of stuff here:

react/packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js

Lines 586 to 637 in 9941cba

	it('dispatches events to the last committed props', async () => {
	const View = createReactNativeComponentClass('RCTView', () => ({
	validAttributes: {},
	uiViewClassName: 'RCTView',
	directEventTypes: {
	topTouchStart: {
	registrationName: 'onTouchStart',
	},
	},
	}));
	const touchStart = jest.fn();
	const touchStart2 = jest.fn();
	await act(() => {
	ReactFabric.render(<View onTouchStart={touchStart} />, 11);
	});
	expect(nativeFabricUIManager.createNode.mock.calls.length).toBe(1);
	expect(nativeFabricUIManager.registerEventHandler.mock.calls.length).toBe(
	1,
	);
	const [, , , , instanceHandle] =
	nativeFabricUIManager.createNode.mock.calls[0];
	const [dispatchEvent] =
	nativeFabricUIManager.registerEventHandler.mock.calls[0];
	const touchEvent = {
	touches: [],
	changedTouches: [],
	};
	expect(touchStart).not.toBeCalled();
	dispatchEvent(instanceHandle, 'topTouchStart', touchEvent);
	expect(touchStart).toBeCalled();
	expect(touchStart2).not.toBeCalled();
	await act(() => {
	ReactFabric.render(<View onTouchStart={touchStart2} />, 11);
	});
	// Intentionally dispatch to the same instanceHandle again.
	dispatchEvent(instanceHandle, 'topTouchStart', touchEvent);
	// The current semantics dictate that we always dispatch to the last committed
	// props even though the actual scheduling of the event could have happened earlier.
	// This could change in the future.
	expect(touchStart2).toBeCalled();
	});

Would it be possible to write a similar regression test at this level?

[gaearon]

I've only now seen #26321. I'm a bit concerned about it. This {node, canonical} structure was there for many years, since the beginning of Fabric. I see we've added four new fields to it, so it's four new fields for every single "instance" in the tree. Though I see we were able to remove one from the class, it's still three new fields in an object that exists for every node. Similarly we've added four new object property reads to cloneInstance.

These are extremely hot paths and commonly used objects. Have we verified that this change does not change the perf in a meaningful way, including memory usage on a large tree or perf on a constrained device? I'm a little uneasy about such a significant change to our most common data structure.

[javache]

These changes seems reasonable to address the bug you describe, but it sounds we may want to have another round of discussion on #26321 to review whether there's any performance concerns.

Any chance we could cover this behaviour with a new unit test?

[rubennorte]

Would it be possible to write a similar regression test at this level?

That's the test I tried to modify to test this behavior.

Any chance we could cover this behaviour with a new unit test?

Yes, I'll give it another try.

[rubennorte]

Thanks for the review, @gaearon and @javache !

Though I see we were able to remove one from the class, it's still three new fields in an object that exists for every node. Similarly we've added four new object property reads to cloneInstance.

I think the important part of this is the removal from the public class, which I'd expect to be neutral for performance and way better for encapsulation.

These are extremely hot paths and commonly used objects. Have we verified that this change does not change the perf in a meaningful way, including memory usage on a large tree or perf on a constrained device? I'm a little uneasy about such a significant change to our most common data structure.

This is only used for host instances, not all fibers in the tree. I don't think having 3 additional properties in an object is going to make that much a difference, especially considering the logic we have around prop diffing that's an order of magnitude more complex.

I'm also planning to make publicInstance lazy so we only create public instances for nodes when they're first needed, which should not only make this neutral but potentially provide a performance win.

[rubennorte]

I ended up doing a partial revert of my refactor in #26321. I preserved the concept of canonical that was shared by all instances, but I moved the public instance to be a field in that object instead of the object itself. That will use 1 additional object temporarily until I make public instances lazy, which should be fine for now.

I think the main problem with my previous approach was that some things that were meant to be shared by all fibers was moved to the fiber itself, and that introduced the series of issues we've seen. This new version should make that right while accomplishing the goal of decoupling that internal state from the public instance provided to users via refs.

[javache]

Why do we need a null-check here? The types claim these are all non-nullable.

[rubennorte]

I don't remember why I did the original check (should've added a comment) but there's some code like this for compatibility with Paper, so types don't match 1:1. I'll double check.

[javache]

Should we rename these methods? It's already exported as getNodeFromInstance

[rubennorte]

I still need to do some more work on the react repo for this. Can do that in a following PR.

[javache]

This was previously using a private property on ReactFabricHostComponent, right? This seems to be the only place we need to access internalInstanceHandle.

[rubennorte]

This was previously using a private property on ReactFabricHostComponent, right?

Yeah.

This seems to be the only place we need to access internalInstanceHandle.

There's another one in ReactFabricComponentTree. What are you suggesting?

[javache]

Just trying to see if we can avoid the prop and re-use the existing one, but since this is now on canonical, it probably doesn't matter as much.

[rubennorte]

We could use symbol properties on the public instance to avoid creating new properties and keep encapsulation, but I want to make public instances lazy and that would prevent me from doing it.

[react-sizebot]

Comparing: eaccf27...abf3094

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	157.44 kB	157.44 kB	=	50.15 kB	50.16 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	159.03 kB	159.03 kB	=	50.66 kB	50.67 kB
facebook-www/ReactDOM-prod.classic.js	=	541.47 kB	541.47 kB	=	96.42 kB	96.42 kB
facebook-www/ReactDOM-prod.modern.js	=	525.20 kB	525.20 kB	=	93.95 kB	93.95 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against abf3094

[javache]

Suggested change

	if (tag === undefined) {
	if (tag === undefined && nativeInstance.canonical) {

[rubennorte]

@gaearon please feel free to still comment here. I can open a follow-up PR in case you have any more concerns after my last changes.