Warn for callback refs on functional components (Stack + Fiber) #8635

iamdustan · 2016-12-24T04:56:40Z

For the changelog, this also implements #7267 in Stack.
It implements it for Fiber too, in addition to other Stack-related Fiber warnings and invariants.

==========

While working on the fiber implementation of ReactTestRenderer I came across this warning which is not yet implemented for fiber.

Initial questions:

Are there any areas of the fiber impl where errors and warnings are expected to live? This is the first instance added to BeginWork which I think may indicate this warning should exist elsewhere
This doesn’t actually stop any ref from being attached. How should that be done?
A related test seems to show refs throwing an error if used on a top-level SFC. Should that exist in the same area of the code or elsewhere?

gaearon · 2016-12-24T17:04:53Z

src/renderers/shared/fiber/ReactFiberBeginWork.js

+        warning(
+          !!workInProgress.ref,
+          'Stateless function components cannot be given refs ' +
+          '(See ref "' + workInProgress.ref._stringRef + '" in ' +


This fails for non-string refs, right?

Yeah, I opened this early for feedback (thanks, btw!)

I didn't see a test case for what should happen in the function ref case already. assuming there isn't a current message for that case what should it be? I don't think it will be too bad to add that case (I'm fiber at least. Will need to look into amending stack)

There is no test case because we silently give you null. It's bad though.

I'd prefer we keep warning parity for now. So if we add it to Fiber we should also backport it to Stack. See #8635 (comment) for context.

gaearon · 2016-12-24T17:10:13Z

This is the first instance added to BeginWork which I think may indicate this warning should exist elsewhere

This looks fine to me. It’s just because we don’t have all warnings yet. A bunch of class-related warnings are in ClassComponent but those functions are called from begin phase as well.

This doesn’t actually stop any ref from being attached. How should that be done?

Is behavior different from Stack? AFAIK it also attaches a ref, just calls it with null.

A related test seems to show refs throwing an error if used on a top-level SFC. Should that exist in the same area of the code or elsewhere?

Which test are you referring to? I think it would be easier to discuss once some tests start passing. We can always change where we emit the warnings but we don’t want to choose before we cover the actual use cases.

gaearon · 2016-12-24T17:11:32Z

Related: it would be great to warn for ref callbacks too. They don't work for functional components either. However even Stack doesn't warn about this. Maybe this is out of scope of this PR though, but up to you. Previous attempt was in #7272.

gaearon · 2016-12-24T17:14:49Z

src/renderers/shared/fiber/ReactFiberBeginWork.js

+          'Stateless function components cannot be given refs ' +
+          '(See ref "' + workInProgress.ref._stringRef + '" in ' +
+          getComponentName(workInProgress) + ' created by ' +
+          getComponentName(workInProgress.return) + '). ' +


This is wrong I think. return will be parent, not owner. For example in <div><Foo /></div>, Foo's return is div.

I think you should use ReactDebugCurrentFiber.getCurrentFiberOwnerName() instead. And in fact maybe you can use ReactDebugCurrentFiber.getCurrentFiberStackAddendum() to add a nice stack instead of "see ref". You could change this in Stack too by using ReactComponentTreeHook.getStackAddendumByID(this._debugID).

Perfect!
To be clear, the tasks are:

update the current test case to have the stack addendum rather than just the owner name

update both stack and fiber impls for that behavior.

Let's change https://github.com/facebook/react/blob/master/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js#L149 to put component in a div to make sure we're printing owner rather than parent name.

It seems as though ReactDebugCurrentFiber.getCurrentFiberStackAddendum() and ReactComponentTreeHook.getStackAddendumByID(this._debugID) return slightly different stacks.

Fiber:

"Warning: Stateless function components cannot be given refs. Attempts to access this ref will fail. in StatelessComponent (at ReactStatelessComponent-test.js:156) in Parent (at ReactStatelessComponent-test.js:160)"

Stack

"Warning: Stateless function components cannot be given refs Attempts to access this ref will fail. in Parent (at ReactStatelessComponent-test.js:160)"

This is with a render method of:

render() { return <div><StatelessComponent name="A" ref="stateless"/></div>; }

Is this acceptable deviance? If yes should I just make the test loose enough to assert on the commonalities? If not how do you want me to proceed?

iamdustan · 2016-12-24T17:26:17Z

This PR gets the following test to pass:
https://github.com/facebook/react/blob/master/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js#L149

This is the throw test that still isn't passing I was referring to.
https://github.com/facebook/react/blob/master/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js#L137

gaearon · 2016-12-24T17:33:16Z

This is the throw test that still isn't passing I was referring to.
https://github.com/facebook/react/blob/master/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js#L137

This is a test for a different unrelated case. It says that you can't use string refs inside SFCs. What you're adding support for is that you can't use string refs on SFCs.

iamdustan · 2016-12-24T17:43:25Z

Ah. Gotcha. Totally misunderstood that test. 👍

iamdustan · 2016-12-27T03:08:45Z

Alright, updated accordingly.

Commits:

first commit updates stack reconciler to use stack in warning.
add support for fiber warning on string refs in the same way
warn for function refs in stack and fiber
run the record-tests script

I had to update ReactIncrementalSideEffects-test.js to swallow the new warning to keep the test from failing. Behavior is the same in that. To support warning for the callback ref in stack I pulled the CompositeTypes enum out of ReactCompositeComponent.js to share between that file and the attach ref file.

gaearon · 2016-12-27T22:29:38Z

src/renderers/shared/fiber/ReactFiberBeginWork.js

+        }
+
+        warning(
+          !workInProgress.ref,


One unfortunate thing related to our warning transformation is that getCurrentFiberOwnerName() runs regardless of this condition. Since this is a hot path in dev, can you please hide it behind a check?

if (workInProgress.ref != null) { ... warning(false, ...) }

Yes, this code is funny.

haha. I’ve seen this type of thing a few times. Now I understand why. :)

gaearon · 2016-12-27T22:30:45Z

src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js

@@ -1069,7 +1069,7 @@ describe('ReactIncrementalSideEffects', () => {
  });

  it('invokes ref callbacks after insertion/update/unmount', () => {
-
+    spyOn(console, 'error');


If the warning wasn't intentional in this test we should fix the reason it occurs. If it was intentional we should assert on the console.error message.

I think the warning is expected. There is a stateless component in this test that should fire a warning. I added the same warning spy that is elsewhere to this test.

gaearon · 2016-12-27T22:41:36Z

src/renderers/shared/stack/reconciler/ReactRef.js

+        'Attempts to access this ref will fail.%s%s',
+        info,
+        ReactComponentTreeHook.getStackAddendumByID((owner || component)._debugID)
+      );


Why do we have almost identical code here and in CompositeComponent? Why can't it live in one place?

gaearon · 2016-12-27T22:42:02Z

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

+        'Warning: Stateless function components cannot be given refs. ' +
+        'Attempts to access this ref will fail. Check the render method ' +
+        'of `Parent`.\n' +
+        '    in Parent (at **)'


Why is there not enough information? Do you have a theory explaining this?

gaearon

I don't quite understand why the warning is in two places but you should be able to consolidate the occurrences.

The string ref code path is ReactRef.attachRef -> ReactOwner.addComponentAsRefTo -> ReactCompositeComponent.attachRef.

The callback ref code path is ReactRef.attachRef (no middlemen).

So it seems like ReactRef.attachRef runs in both cases, and is a better place to put the warning. You just need to lift it from the ref type condition so that it covers both cases.

Once you start using component._debugID there, you should get the full stack. Then you can remove the branches from the warning checks.

Finally please either fix the test in Incremental or add an expectation for which exactly warning should fire there (if it's justified) so that we know if we accidentally regress it and it causes a different warning.

gaearon · 2016-12-27T22:50:31Z

src/renderers/shared/stack/reconciler/ReactCompositeComponent.js

+        'Stateless function components cannot be given refs. Attempts to ' +
+        'access this ref will fail.%s%s',
+        info,
+        ReactComponentTreeHook.getStackAddendumByID(this._debugID)


I think you want component._debugID, not this._debugID here. Because you want the stack for the child component (the one whose ref we're trying to attach) rather than the owner (the one who's going to own the ref).

gaearon · 2016-12-27T22:53:08Z

src/renderers/shared/stack/reconciler/ReactRef.js

+        'Stateless function components cannot be given refs. ' +
+        'Attempts to access this ref will fail.%s%s',
+        info,
+        ReactComponentTreeHook.getStackAddendumByID((owner || component)._debugID)


Similarly you want the component's debugID, not the owner's. I think component is guaranteed to exist since we're calling component.getPublicInstance() on the next line.

gaearon · 2016-12-27T23:01:46Z

Actual review here: #8635 (review). I don't see why GH puts outdated comments after it.

iamdustan · 2016-12-28T15:38:20Z

src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js

+    expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe(
+      'Warning: Stateless function components cannot be given refs. ' +
+      'Attempts to access this ref will fail. Check the render method ' +
+      'of `FunctionalComponent`.\n' +


Thank you for the thorough review, @gaearon. This is my own remaining question. While this test passes this looks wrong to me. (The same set of assertions fails in ReactStatelessComponent with fiber on).

I would expect the ownerName to be Foo rather than FunctionalComponent. If I cahnge the getCurrentFiberOwnerName() to return the fiber._debugOwner for FunctionalComponents then I get Foo...

I’m going to push one more commit with that change for review since it looks like you landed that getCurrentFiberOwnerName change yesterday.

iamdustan · 2016-12-28T16:15:11Z

src/renderers/shared/fiber/ReactDebugCurrentFiber.js

      case ClassComponent:
        return getComponentName(fiber);
+      case FunctionalComponent:


If I put FunctionalComponent directly with HostComponent where it returns null when there is no fiber._debugOwner then three following tests move from the fiber success to fiber failure:

src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js -* catches reconciler errors in a boundary during mounting -* catches reconciler errors in a boundary during update src/renderers/shared/shared/__tests__/ReactComponent-test.js -* includes owner name in the error about badly-typed elements

All three of those tests specifically; include a FunctionalComponent returning const X = undefined; return <X />.

Why would you move them together with HostComponent? I'm not sure I'm following.

oh, idk. It was just a thing I tried and wanted to document. I tried it in an attempt to not give FunctionalComopnents their own case.

gaearon · 2016-12-30T20:23:57Z

src/renderers/shared/fiber/ReactDebugCurrentFiber.js

      case FunctionalComponent:
+        if (fiber._debugOwner != null) {
+          return getComponentName(fiber._debugOwner);
+        }


This doesn't look right to me. There's nothing special about functional components from the warning message perspective as far as I know.

If I remove this line then the following three tests fail:

src/renderers/shared/fiber/__tests__/React IncrementalSideEffects-test.js -* invokes ref callbacks after insertion/update/unmount src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js * should update stateless component * should unmount stateless component * should pass context thru stateless component -* should warn when given a string ref -* should warn when given a function ref

They all fail for the same reason. This is an example of the results from the current stack reconciler.

expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( 'Warning: Stateless function components cannot be given refs. ' + 'Attempts to access this ref will fail. Check the render method ' + 'of `Foo`.\n' + ' in FunctionalComponent (at **)\n' + ' in div (at **)\n' + ' in Foo (at **)' );

If I remove this line then Fiber ends up with:

expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( 'Warning: Stateless function components cannot be given refs. ' + 'Attempts to access this ref will fail. Check the render method ' + ~ 'of `FunctionalComponent`.\n' + ' in FunctionalComponent (at **)\n' + ' in div (at **)\n' + ' in Foo (at **)' );

tldr; without this line Fiber ends up logging the FunctionalComponents name in the same location that stack logs the FunctionalComponents owners name.

Right, but this doesn’t mean functional components are special compared to class components, does it? I think this might mean we need separate methods, e.g. getCurrentFiberOwnerName and getCurrentFiberName. Or something like this. Maybe we’re wrong in using getCurrentFiberOwnerName for both of these errors, and one of them needs something different.

I haven’t really thought about this much, just giving you ideas to think about. It’s more likely that these errors need different Fibers in messages, than that functional components are special.

iamdustan · 2017-01-09T18:18:39Z

Hey @gaearon, just got back from being out of town on holiday last week. I see you pushed a few commits to this. Where does this stand currently? Want me to pick it back up and explore your last comments a bit further or were you intending on continuing to push it?

I have a lot of work to catch up on early this week, but will pick it back up later this week unless you say otherwise.

gaearon · 2017-01-09T18:51:20Z

Where does this stand currently? Want me to pick it back up and explore your last comments a bit further or were you intending on continuing to push it?

I’m trying to figure out the proper fix and want to get it merged today.

… instead

We don't have a test for it. It's easy to mess it up and print the wrong thing so instead of verifying it I'll just remove this bit.

This happened to work, but it is just a coincidence. This change didn’t really match what the function was supposed to be doing. I’m not sure what the correct fix would be yet so this commit breaks tests.

This passes in Stack. It helps ensure we supply the correct owner name.

This brings the Fiber behavior in line with how Stack does it. The Fiber test was passing accidentally but broke down in more complicated cases (such as when we have an <Indirection> between the owner and the failing element). Now we can also remove the weird cases from getCurrentFiberOwnerName() that didn't really make sense but helped get the (incomplete) tests pass in the past.

iamdustan · 2017-01-09T21:16:33Z

src/renderers/shared/fiber/ReactDebugCurrentFiber.js

-        return null;
-      default:
-        return null;
+    if (fiber._debugOwner != null) {


👍 I suspected this was going to be reverted from your last comments.

iamdustan · 2017-01-09T21:22:03Z

src/renderers/shared/fiber/ReactFiber.js

@@ -328,7 +333,7 @@ exports.createFiberFromText = function(content : string, priorityLevel : Priorit
  return fiber;
 };

-function createFiberFromElementType(type : mixed, key : null | string) : Fiber {
+function createFiberFromElementType(type : mixed, key : null | string, debugOwner : null | Fiber | ReactInstance) : Fiber {


so the resulting fix is passing the debugOwner through the createFiberFrom* functions explicitly. Yeah?

I couldn’t find a better solution.

Using the "current fiber" was wrong here because the thing being created is the current fiber, but it's not created yet.

I think that makes sense. The two ideas i was going to explore was to either create something like getDebugFiberWorkInProgress or floundering my way through something like what you did.

What were the other solutions you tried that didn’t work out?

I didn't really try anything else.

create something like getDebugFiberWorkInProgress

The problem here is that the fiber isn't created yet so you wouldn't be able to set a reference.

gaearon · 2017-01-09T21:24:26Z

Okay, let's wait for CI on this and then merge.
Thanks for working on this!

iamdustan · 2017-01-09T21:25:52Z

...and thanks for finishing it!

gaearon · 2017-01-10T17:54:22Z

Re-tagged as minor since it's technically a new warning.

facebook-github-bot added GH Review: review-needed CLA Signed labels Dec 24, 2016

gaearon reviewed Dec 24, 2016

View reviewed changes

iamdustan force-pushed the statelss-component branch from e1428ad to 5c8a52c Compare December 27, 2016 03:04

iamdustan force-pushed the statelss-component branch from 5c8a52c to 0d3858e Compare December 27, 2016 03:10

iamdustan mentioned this pull request Dec 27, 2016

[Fiber] Include owner in invalid element type invariant #8637

Merged

iamdustan force-pushed the statelss-component branch from 0d3858e to 51a1ee0 Compare December 27, 2016 16:55

gaearon reviewed Dec 27, 2016

View reviewed changes

gaearon requested changes Dec 27, 2016

View reviewed changes

iamdustan force-pushed the statelss-component branch 2 times, most recently from b87524f to a42306a Compare December 28, 2016 15:34

iamdustan commented Dec 28, 2016

View reviewed changes

iamdustan force-pushed the statelss-component branch 3 times, most recently from dd54156 to 21aee21 Compare December 28, 2016 16:08

iamdustan commented Dec 28, 2016

View reviewed changes

gaearon reviewed Dec 30, 2016

View reviewed changes

gaearon mentioned this pull request Jan 9, 2017

Warn for functional refs on stateless functional components #7272

Closed

iamdustan added 2 commits January 9, 2017 19:27

Stateless refs: update warning to use component stack

943c56e

Fiber: warn for refs on SFCs

178b4e7

iamdustan and others added 3 commits January 9, 2017 19:27

ReactDebugCurrentFiber maybe FunctionalComponents should act this way…

7ac1fdd

… instead

(chore) scripts/fiber/record-tests

a6752fd

Add component._compositeType to ReactInstance Flow definition

ff442b6

gaearon force-pushed the statelss-component branch from 6b898d2 to 1367f6e Compare January 9, 2017 19:50

gaearon added 3 commits January 9, 2017 20:15

Don't handle 'stack inside fiber' case in the warning

028c651

We don't have a test for it. It's easy to mess it up and print the wrong thing so instead of verifying it I'll just remove this bit.

Revert the change to getCurrentFiberOwnerName

dc1c3f8

This happened to work, but it is just a coincidence. This change didn’t really match what the function was supposed to be doing. I’m not sure what the correct fix would be yet so this commit breaks tests.

Add component indirection to the tests using owner name

c61ad14

This passes in Stack. It helps ensure we supply the correct owner name.

gaearon force-pushed the statelss-component branch from 1367f6e to fafb8ef Compare January 9, 2017 20:38

gaearon added 2 commits January 9, 2017 21:06

Fiber should throw on a string ref inside functional component

a070e9a

gaearon force-pushed the statelss-component branch from fafb8ef to a070e9a Compare January 9, 2017 21:07

gaearon approved these changes Jan 9, 2017

View reviewed changes

gaearon changed the title ~~Fiber: warn for refs on SFCs~~ Warn for callback refs on functional components (Stack + Fiber) Jan 9, 2017

gaearon added this to the 15-lopri milestone Jan 9, 2017

gaearon added the semver-patch label Jan 9, 2017

iamdustan commented Jan 9, 2017

View reviewed changes

gaearon merged commit 90294ea into facebook:master Jan 9, 2017

gaearon mentioned this pull request Jan 10, 2017

Dedupe warnings about refs on functional components #8739

Merged

iamdustan deleted the statelss-component branch January 10, 2017 16:42

gaearon added semver-minor and removed GH Review: review-needed labels Jan 10, 2017

Maximilianos mentioned this pull request Jan 20, 2017

Improve warnings about refs on functional components during runtime and in docs #7267

Closed

gaearon mentioned this pull request Jan 24, 2017

React 16 Umbrella ☂️ (and 15.5) #8854

Closed

bvaughn mentioned this pull request Aug 1, 2017

React 16 RC #10294

Closed

Warn for callback refs on functional components (Stack + Fiber) #8635

Warn for callback refs on functional components (Stack + Fiber) #8635

Conversation

iamdustan commented Dec 24, 2016 • edited by gaearon Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Dec 24, 2016

gaearon commented Dec 24, 2016

Choose a reason for hiding this comment

iamdustan Dec 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamdustan commented Dec 24, 2016

gaearon commented Dec 24, 2016

iamdustan commented Dec 24, 2016

iamdustan commented Dec 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Dec 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamdustan commented Jan 9, 2017

gaearon commented Jan 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Jan 9, 2017

iamdustan commented Jan 9, 2017

gaearon commented Jan 10, 2017

iamdustan commented Dec 24, 2016 •

edited by gaearon

Loading

iamdustan Dec 24, 2016 •

edited

Loading

gaearon commented Dec 27, 2016 •

edited

Loading