React.pure automatically forwards ref

[sophiebits]

We're not planning to encourage legacy context, and without this change, it's difficult to use pure+forwardRef together. We could special-case pure(forwardRef(...)) but this is hopefully simpler.

React.pure(function(props, ref) {
  // ...
});

[gaearon]

Neat. I like this.

[acdlite]

Need to fix this for forwardRef, too,

[acdlite]

Thanks! Need to fix this for forwardRef, too, but we can do that in a separate PR.

[sophiebits]

Looks like we do already:

react/packages/react-reconciler/src/ReactFiberBeginWork.js

Lines 187 to 208 in 0af8199

	function updateForwardRef(
	current: Fiber | null,
	workInProgress: Fiber,
	type: any,
	nextProps: any,
	renderExpirationTime: ExpirationTime,
	) {
	const render = type.render;
	const ref = workInProgress.ref;
	if (hasLegacyContextChanged()) {
	// Normally we can bail out on props equality but if context has changed
	// we don't do the bailout and we have to reuse existing props instead.
	} else if (workInProgress.memoizedProps === nextProps) {
	const currentRef = current !== null ? current.ref : null;
	if (ref === currentRef) {
	return bailoutOnAlreadyFinishedWork(
	current,
	workInProgress,
	renderExpirationTime,
	);
	}
	}

though I'm not sure we need to check legacy context there?

[acdlite]

Problem is we never hit this code path because of:

react/packages/react-reconciler/src/ReactFiberBeginWork.js

Lines 1209 to 1217 in 0af8199

	if (current !== null) {
	const oldProps = current.memoizedProps;
	const newProps = workInProgress.pendingProps;
	if (
	oldProps === newProps &&
	!hasLegacyContextChanged() &&
	(updateExpirationTime === NoWork ||
	updateExpirationTime > renderExpirationTime)
	) {

[sizebot]

Details of bundled changes.

Comparing: 0af8199...08e62e4

scheduler

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
scheduler.development.js	n/a	n/a	0 B	19.17 KB	0 B	5.74 KB	UMD_DEV
scheduler.production.min.js	n/a	n/a	0 B	3.16 KB	0 B	1.53 KB	UMD_PROD

Generated by 🚫 dangerJS

[sophiebits]

Ironically had to figure out how to compose these for the sole purpose of testing this change.

[sebmarkbage]

I don't think we should take this lightly.

It's true that one proposal was to always pass the ref as the second argument for all function components.

However, another proposal was to instead not special case it and move it to props.

The forwardRefs mechanism has the benefit is that we can treat it as a legacy API that unwraps props. For example, extract it from props and instead pass it as a second arg. This would be a slow operation since it would loop over and clone (just like createElement does today).

This PR makes it so that we'd have to keep doing that, not just on the "legacy" non-idiomatic forwardRef API but also for the best-practice pure API too.

This PR is effectively saying that we've decided to always pass the second argument ref and keep special casing ref.

[sophiebits]

Fair enough. I was under the impression that ref as the second arg being the best practice had been decided when I was out.