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

Render during reconciliation causes NPE #6027

Closed
sambev opened this issue Feb 12, 2016 · 5 comments
Closed

Render during reconciliation causes NPE #6027

sambev opened this issue Feb 12, 2016 · 5 comments

Comments

@sambev
Copy link

sambev commented Feb 12, 2016

I have a view with a controlled select on it. When the onChange event fires, the resulting function actually changes the view, removing the select. Then I get this error:

Uncaught TypeError: Cannot set property 'pendingUpdate' of null

Reproduced here:
https://jsfiddle.net/wntgpwcx/5/ (react 0.14.7)

I also noticed I don't get the same error in an older version (0.13.1)
https://jsfiddle.net/wntgpwcx/6/

The line that throws is in ReactDOMSelect L183:

function _handleChange(event) {
  var props = this._currentElement.props;
  // _wrapper state is not null here
  var returnValue = LinkedValueUtils.executeOnChange(props, event);

  this._wrapperState.pendingUpdate = true; // _wrapperState is null after the executeOnChange happens
  ReactUpdates.asap(updateOptionsIfPendingUpdateAndMounted, this);
  return returnValue;
}

I was able to see a fix for a similar situation here:
https://github.com/facebook/react/pull/4624/files

I don't know if a similar check could be applied here, or maybe I just shouldn't remove my select as a result of an onChange event?

@jimfb jimfb changed the title Select onChange triggers Uncaught TypeError: Cannot set property 'pendingUpdate' of null Render during reconciliation causes NPE Feb 12, 2016
@jimfb
Copy link
Contributor

jimfb commented Feb 12, 2016

Probably affects unstable_subtree also.

I don't think we need a fix this for the v15 milestone since it was a regression introduced in 0.14 and this is the first we're hearing about it, but it is a valid bug.

@sambev
Copy link
Author

sambev commented Feb 12, 2016

@jimfb Would the check in the PR I linked be a sufficient check to add? I would be more than happy to add it in myself.

@jimfb
Copy link
Contributor

jimfb commented Feb 12, 2016

@sambev Sorry, I'm not sure I understand your question. The PR you linked was merged, but the problem still exists on master. What specifically would be the fix here?

@sambev
Copy link
Author

sambev commented Feb 12, 2016

@jimfb Sorry, I wasn't that clear. I simply applied the same conditional in that PR in #6028. However, I don't know the full impact of that change.

@jimfb
Copy link
Contributor

jimfb commented Feb 13, 2016

Fixed in #6028 and verified in http://jsfiddle.net/uptxgjvd/

@jimfb jimfb closed this as completed Feb 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants