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

Fix "null" instead of the component stack in a warning #10915

Merged
merged 4 commits into from
Sep 28, 2017

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Sep 28, 2017

Fixes #10831.

(Note: the warning itself is still legit. This just fixes it to print the component stack instead of null in some cases.)

See individual commit messages for more info on what we did wrong and why this helps.

@sebmarkbage
Copy link
Collaborator

There are so many places this gets reset. I have no confidence in whether this was needed or not and how it's supposed to work. 😕

@sebmarkbage
Copy link
Collaborator

Are there other cases this could be null? Shouldn't we print a better message than "null" incase it gets to be null.

It should only be reset when we stop doing work.
Otherwise, any warnings after the reset will lose the component stack.

The reset in getMaskedContext() was completely unnecessary.
It is always called with the same fiber as the current work in progress.
Therefore, I add a DEV-only warning assertion to ensure we don't regress, and remove the reset.

The reset in processChildContext() is necessary because it can be called outside of reconciliation.
Unfortunately, we have to keep this hack in until we can remove unstable_renderSubtreeIntoContainer().
To work around it, I restore the previous fiber instead of resetting.
@gaearon
Copy link
Collaborator Author

gaearon commented Sep 28, 2017

Okay, I can restructure it a bit to make it clearer. I think it's actually not complicated, but the unstable_renderIntoSubtree special case makes it confusing.

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

I'll trust the unit test coverage for now. :)

@gaearon
Copy link
Collaborator Author

gaearon commented Sep 28, 2017

Why don't you just trust me, Sebastian

@reactjs-bot
Copy link

reactjs-bot commented Sep 28, 2017

Deploy preview failed.

Built with commit 3afc7b4

https://app.netlify.com/sites/reactjs/deploys/59cd4c7cdf995340c19c37fa

These are two distinct actions.
This helps make it clearer when we're actually changing the current pointer.

I'm also removing an overengineered hack I previously added for unstable_renderSubtreeIntoContainer. It's not necessary now that we don't null the pointer all the time.
This makes the code more straightforward.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants