Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Creating element with ref in constructor does not throw error in dev mode (#9635) #10025

Merged
merged 1 commit into from
Aug 15, 2017

Conversation

iansu
Copy link
Contributor

@iansu iansu commented Jun 22, 2017

Currently just a failing test to show that this issue does exist. Next step is to fix the issue. See issue #9635.

@iansu
Copy link
Contributor Author

iansu commented Jun 22, 2017

I've managed to fix this bug but, to be perfectly honest, I don't fully understand what's going on here. Previously when constructing a component in dev mode ReactCurrentOwner.current was set to this. I just updated that check so that ReactCurrentOwner.current is only set when __DEV__ = true and the component doesn't have a constructor. If the component does have a constructor then it follows the same code path as it would when __DEV__ = false. That makes my failing test pass and doesn't break any other tests. 🤷‍♂️

@gaearon
Copy link
Collaborator

gaearon commented Jun 23, 2017

Can you also check if this ever worked with previous versions of React? Specifically 0.13 and 0.14?

@iansu
Copy link
Contributor Author

iansu commented Jun 23, 2017

@gaearon I had a bit of trouble getting 0.13 installed but I think I got it working enough to run the tests. It looks like the error is thrown in 0.13 when __DEV__ = true and is not thrown in 0.14.

@aweary aweary mentioned this pull request Jun 26, 2017
12 tasks
ReactTestUtils.renderIntoDocument(<RefTest />);
}).toThrowError();

__DEV__ = originalDev;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be in a finally block so that it happens regardless.


expect(function() {
ReactTestUtils.renderIntoDocument(<RefTest />);
}).toThrowError();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to assert a specific error message here. I don't think Fiber currently has a nice message in this case. Can you look into it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally tried checking for the specific error message. I copy/pasted the message but it said actual vs. expected message didn't match, even though they appeared identical. I'll give it another try.

@iansu
Copy link
Contributor Author

iansu commented Jun 29, 2017

I've updated the tests to check for the specific error message. This breaks the tests in Fiber since the error message is different. @gaearon you're correct that this error does not have a nice message in Fiber, it just results in a TypeError. Here's the output:

  ● creating element with ref in constructor › throws an error when __DEV__ = false

    expect(function).toThrowError(string)
    
    Expected the function to throw an error matching:
      "addComponentAsRefTo(...): Only a ReactOwner can have refs. You might be adding a ref to a component that was not created inside a component's `render` method, or you have multiple copies of React loaded (details: https://fb.me/react-refs-must-have-owner)."
    Instead, it threw:
      TypeError: ref is not a function      
          at commitAttachRef (src/renderers/shared/fiber/ReactFiberCommitWork.js:563:11)
          at commitAllLifeCycles (src/renderers/shared/fiber/ReactFiberScheduler.js:387:9)
          at commitAllWork (src/renderers/shared/fiber/ReactFiberScheduler.js:511:11)
          at completeUnitOfWork (src/renderers/shared/fiber/ReactFiberScheduler.js:666:11)
          at performUnitOfWork (src/renderers/shared/fiber/ReactFiberScheduler.js:698:14)
          at workLoopSync (src/renderers/shared/fiber/ReactFiberScheduler.js:805:24)
          at workLoop (src/renderers/shared/fiber/ReactFiberScheduler.js:837:9)
          at performWork (src/renderers/shared/fiber/ReactFiberScheduler.js:894:11)
          at scheduleUpdate (src/renderers/shared/fiber/ReactFiberScheduler.js:1323:19)
          at scheduleTopLevelUpdate (src/renderers/shared/fiber/ReactFiberReconciler.js:231:5)
          at Object.updateContainer (src/renderers/shared/fiber/ReactFiberReconciler.js:267:7)
          at src/renderers/dom/fiber/ReactDOMFiberEntry.js:507:19
          at Object.unbatchedUpdates (src/renderers/shared/fiber/ReactFiberScheduler.js:1418:14)
          at renderSubtreeIntoContainer (src/renderers/dom/fiber/ReactDOMFiberEntry.js:506:17)
          at Object.render (src/renderers/dom/fiber/ReactDOMFiberEntry.js:553:12)
          at Object.renderIntoDocument (src/renderers/dom/test/ReactTestUtilsEntry.js:153:21)
          at src/renderers/__tests__/refs-test.js:520:24
          at Object.<anonymous> (src/renderers/__tests__/refs-test.js:521:10)
      
      at Object.<anonymous> (src/renderers/__tests__/refs-test.js:521:10)

@iansu
Copy link
Contributor Author

iansu commented Jun 29, 2017

I have a potential fix for this in Fiber. Currently on line 559 of ReactFiberCommitWork.js we check if ref is not null. I've modified that line to check if ref is a function, since that's how it's used inside that block. If it's not a function I'm just displaying the same error message as the non-Fiber version. Let me know if you'd like me to commit this fix.

@iansu
Copy link
Contributor Author

iansu commented Jul 13, 2017

@gaearon I've made the requested changes here and pushed a commit but I'm not sure if there's some way I can request another review or otherwise confirm that I've made the changes. I assume you've just got your hands full but I want to make sure you're not waiting on me. Also let me know about the Fiber fix in my previous comment.

@nhunzaker nhunzaker mentioned this pull request Aug 10, 2017
11 tasks
@gaearon
Copy link
Collaborator

gaearon commented Aug 12, 2017

Sorry this slipped through the cracks. We were focused on getting 16 beta out.

Can you rebase on master? I think the change to Fiber code is now not necessary because we have a better check there.

@iansu
Copy link
Contributor Author

iansu commented Aug 12, 2017

I rebased upstream/master into my branch and now it's showing all the commits that have happened since then as part of my pull request. I'm not exactly sure how to fix that. 😬

Looking at master I don't see any new checks in commitAttachRef. So unless something is happening elsewhere to guarantee that finishedWork.ref is either null or a function and not some other type then I think this error can still happen.

@iansu
Copy link
Contributor Author

iansu commented Aug 12, 2017

I tried running the tests I added for this issue with the version of commitAttachRef from master and they fail so this bug does still exist.

Also, no need to apologize, I understand that React 16 is high priority. I want it to be released too. 😀

UPDATE: It looks like the error is being caught but with a different error message which is why the tests are failing.

@iansu
Copy link
Contributor Author

iansu commented Aug 12, 2017

I've reverted the changes in commitAttachRef and updated my tests.

@gaearon
Copy link
Collaborator

gaearon commented Aug 14, 2017

The extra commits look weird. Let's just do

git fetch origin # assuming it points to main repo
git merge origin/master
git reset --soft origin/master
git add .
git commit

And then force push. This will turn it into a single commit on top of current master.

@iansu
Copy link
Contributor Author

iansu commented Aug 14, 2017

origin points to my fork but I've got upstream set to point to the main repo. I will fix this shortly and push up my changes.

… Only set ReactCurrentOwner.current in dev mode when the component has no constructor.
@iansu
Copy link
Contributor Author

iansu commented Aug 14, 2017

Done! GitHub was still showing all the commits on the pull request for a minute there and I thought I effed it up again but it looks good now. Thanks for the help Dan!

@gaearon gaearon merged commit 7f6b940 into facebook:master Aug 15, 2017
@gaearon
Copy link
Collaborator

gaearon commented Aug 15, 2017

Sweet. Thanks.

nhunzaker pushed a commit that referenced this pull request Aug 15, 2017
… Only set ReactCurrentOwner.current in dev mode when the component has no constructor. (#10025)

nhunzaker: I've added an additional line to the ref test that sets the
NODE_ENV invironment variable. This allows the test to pass.
nhunzaker added a commit that referenced this pull request Aug 15, 2017
nhunzaker added a commit that referenced this pull request Aug 15, 2017
nhunzaker pushed a commit that referenced this pull request Aug 15, 2017
… Only set ReactCurrentOwner.current in dev mode when the component has no constructor. (#10025)

nhunzaker: I've added an additional line to the ref test that sets the
NODE_ENV invironment variable. This allows the test to pass.
nhunzaker added a commit that referenced this pull request Aug 15, 2017
nhunzaker added a commit that referenced this pull request Aug 15, 2017
sophiebits pushed a commit that referenced this pull request Sep 26, 2017
… Only set ReactCurrentOwner.current in dev mode when the component has no constructor. (#10025)

nhunzaker: I've added an additional line to the ref test that sets the
NODE_ENV invironment variable. This allows the test to pass.
sophiebits pushed a commit that referenced this pull request Sep 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants