Warn when nesting 15 subtree inside 16 #10434
Conversation
| 'a React 15 tree inside a React 16 tree using ' + | ||
| "unstable_renderSubtreeIntoContainer, which isn't supported. Try to " + | ||
| 'make sure you have only one copy of React (and ideally, switch to ' + | ||
| 'React.unstable_createPortal).', |
There was a problem hiding this comment.
It's currently on ReactDOM.
There was a problem hiding this comment.
Oh weird. Why? Can amend if that's really intentional.
There was a problem hiding this comment.
Intentional because it accepts a DOM node as an argument. Could make one shared around DOM client renderers or SSR compatible that accepts a different argument such as a string query.
|
Did changing the name of |
|
I think we still need a fake _processChildContext method – you'd prefer changing the name _reactInternalInstance and adding a field ._reactInternalInstance = {_processChildContext: throw} on class components? |
|
Yea, I'd prefer that. The possibilities of collision here seems scary. What happens if we don't add the place holder to class instances? |
|
I'm not sure what your last question means. |
| @@ -283,6 +305,7 @@ module.exports = function( | |||
| workInProgress.stateNode = instance; | |||
| // The instance needs access to the fiber so that it can schedule updates | |||
| ReactInstanceMap.set(instance, workInProgress); | |||
| instance._reactInternalInstance = fakeInternalInstance; | |||
There was a problem hiding this comment.
Should I make this DEV-only? Seems safer to have it be in prod too. :(
There was a problem hiding this comment.
DEV only plz. We stick random stuff on classes all over.
There was a problem hiding this comment.
If it's DEV only we at least know that we can remove it at some point. E.g. a minor.
| @@ -26,19 +26,19 @@ var ReactInstanceMap = { | |||
| * supported we can rename it. | |||
| */ | |||
| remove: function(key) { | |||
| key._reactInternalInstance = undefined; | |||
| key._reactInternalInstance16 = undefined; | |||
|
Does this just work with devtools? |
| 'a React 15 tree inside a React 16 tree using ' + | ||
| "unstable_renderSubtreeIntoContainer, which isn't supported. Try " + | ||
| 'to make sure you have only one copy of React (and ideally, switch ' + | ||
| 'to ReactDOM.unstable_createPortal).', |
There was a problem hiding this comment.
nit - we could condense this and then link to docs, which could provide more info.
I think we could always improve the error message in a follow-up commit, if this comes up often for folks.
I'm not sure what that was referring to - it seems like this would work in lgtm but want to let @sebmarkbage give it a final look if possible. |
|
The question is whether the devtools relies on _reactInternalInstance. I checked the code last week and am 90% sure this will work but if you want to double check that the dev tools work if you build this and open a fixture, this is good for merge.
…________________________________
From: Flarnie Marchan <notifications@github.com>
Sent: Sunday, August 13, 2017 11:35:06 AM
To: facebook/react
Cc: Ben Alpert; Author
Subject: Re: [facebook/react] Warn when nesting 15 subtree inside 16 (#10434)
Does this just work with devtools?
I'm not sure what that was referring to - it seems like this would work in DEV mode.
lgtm but want to let @sebmarkbage<https://github.com/sebmarkbage> give it a final look if possible.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#10434 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAAapNloBB952FAGiqKLpWDMc2GBo3kUks5sX0HagaJpZM4O0GBD>.
|
|
Sounds good - I'll do a quick manual test. |
| @@ -26,19 +26,19 @@ var ReactInstanceMap = { | |||
| * supported we can rename it. | |||
| */ | |||
| remove: function(key) { | |||
| key._reactInternalInstance = undefined; | |||
| key._reactInternalFiber = undefined; | |||
There was a problem hiding this comment.
This will break hot reloading (which we just fixed). Do you mind sending a PR to react-deep-force-update to account for this change? Should be as easy as checking both fields.
No description provided.