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

patchComponent: Cannot read property '_unmounted' of null #895

Closed
kanzelm3 opened this issue Feb 27, 2017 · 35 comments
Closed

patchComponent: Cannot read property '_unmounted' of null #895

kanzelm3 opened this issue Feb 27, 2017 · 35 comments

Comments

@kanzelm3
Copy link
Contributor

kanzelm3 commented Feb 27, 2017

Observed Behaviour

Inferno is throwing and uncaught exception Cannot read property '_unmounted' of null. It is happening inside patchComponent when lastVNode.children is null. It is throwing because instance._unmounted is null to be more specific.

if (instance._unmounted) {

Expected Current Behaviour

Inferno should not throw an uncaught exception, but should handle null as children appropriately.

Inferno Metadata

Using inferno/inferno-compat v1.3.0-rc.8

macOS / Chrome

@Havunen
Copy link
Member

Havunen commented Feb 28, 2017

I faced same error when investigated another task. Can you create simple steps to reproduce please.

@Havunen
Copy link
Member

Havunen commented Feb 28, 2017

I am seeing same exception here also:

if (!instance._unmounted) {

@Havunen
Copy link
Member

Havunen commented Feb 28, 2017

There is also third place where this fails instance being null, so somewhere its not setting instance correctly.

@Havunen
Copy link
Member

Havunen commented Mar 1, 2017

Simple steps to reproduce needed...

@kanzelm3 kanzelm3 changed the title pathComponent: Cannot read property '_unmounted' of null patchComponent: Cannot read property '_unmounted' of null Mar 2, 2017
@roastlechon
Copy link

@Havunen @kanzelm3 we currently are not able to provide a simple repository to reproduce the issue sadly.

@Havunen
Copy link
Member

Havunen commented Mar 2, 2017

Well I have good news for you. I have successfully reproduced this and added test scenario for failing patch and unmount: 8eb679a

the scenario is not that simple tho... lol

@roastlechon
Copy link

@Havunen You are awesome! Our code that produces this issue is surrounding something relating to Mapbox. Some what complicated as well.

@Havunen
Copy link
Member

Havunen commented Mar 3, 2017

This will be fixed in next release. Closing.

@Havunen Havunen closed this as completed Mar 3, 2017
@kanzelm3
Copy link
Contributor Author

kanzelm3 commented Mar 9, 2017

@Havunen I have tested this with rc.9 and rc.10 and I still see the issue in both.

@Havunen
Copy link
Member

Havunen commented Mar 9, 2017

@kanzelm3 okay, I need steps to reproduce it.

@kirill-kruchkov
Copy link

kirill-kruchkov commented Mar 10, 2017

Hello! This one reproduces in 1.3.1.

@Havunen
Copy link
Member

Havunen commented Mar 11, 2017

@kirill-kruchkov I can re-open this, but last time I spent 2 full days trying to reproduce it and found one of scenarios when it happens, which is now fixed and tested for regression. If you can reproduce it again using different strategy I'm more than willing to fix it. But I need help reproducing this issue.

@Havunen Havunen reopened this Mar 11, 2017
@nixedmelon
Copy link

I'm getting the same issue in 1.4 when dispatching actions from componentWillReceiveProps. I did not have this issue before with Inferno 1.0.0-beta42, but going back results in the same problem, so it's something that I changed. Still looking into it.

@Havunen
Copy link
Member

Havunen commented Mar 16, 2017

@OMLET can you share source code for that project or create minimal setup to reproduce it?

@nixedmelon
Copy link

nixedmelon commented Mar 17, 2017

@Havunen I'll try and write up a minimal setup. I did some minor testing using inferno redux, and found the following:

  • Component A is connected to store
  • Component B is connected to store
  • Component C within Component B receives store as prop from Component B
  • Component C dispatches an action, which causes A's componentWillReceiveProps to fire.
  • If Component A dispatches an action within componentWillReceiveProps in response to Component C dispatching an action, I get the error.

Below is a screenshot of what I see:

wowza

In the store, I have a value named seek with an initial value of 0. Component C updates this value. Component A, upon seeing this value change from 0, will dispatch an action to reset back to 0. In the screenshot, you can see the initial update works fine, but after dispatching the action from within Component A's componentWillReceiveProps, the state is wrong: the previous seek should be 80.475. At this point, the error occurs.

I'm not sure if this of any help. Again, I'll try and write up a setup for testing - the above is simply what I discovered in the main application.

@Havunen
Copy link
Member

Havunen commented Mar 17, 2017

Okay thanks for update. Can you create external repository to reproduce it?

@kanzelm3
Copy link
Contributor Author

Ok, I have found the root cause of the issue. I'll do my best to explain, but @OMLET is correct it is directly related to dispatching actions from within componentWillReceiveProps in a child component.

Let's say you have component A, a container component wrapped with the connect higher order component from react-redux and component B, a child of Component A that will react to changes in props or state by dispatching an action from within componentWillReceiveProps.

In some other child component of component A, a button is clicked that dispatches an action and causes component A to be patched. During the patch method it synchronously steps through each component in the tree and patches the component. Once it reaches component B, it calls the _updateComponent method and that calls this.componentWillReceiveProps since it is defined.

Inside componentWillReceiveProps a new action is dispatched based on the props for component B changing, which is a synchronous function that results in the listener in react-redux's higher order component being fired. This in turn leads to a call to this.setState from within the Connect higher order component, which has this._syncSetState set to true so it is done synchronously.

The Connect higher order component is wrapping the parent component (component A) and the sync set state triggers that component to be patched synchronously immediately. When this patch happens, the initial patch on component A did not finish and the instances for all of it's children have not been set because instance._lastInput = nextInput; inside of the initial patch set the _lastInput property to nextInput already but the patch for nextInput did not complete. Any component that would have been patched AFTER component B has a null children property due to patch never reaching it.

This is hard to explain, but I hope you understand what I am saying. If you have any questions please let me know and I can try to get a repo up that demonstrates the issue. In order to unit test this, you would have to either use the react-redux connect higher order component or create a HoC that does something similar to create the patch loop that causes the children instances to be null.

@Havunen
Copy link
Member

Havunen commented Mar 18, 2017

ping @trueadm , How should we fix this? It sounds like setState related.

@trueadm
Copy link
Member

trueadm commented Mar 18, 2017

I'm a little confused, componentWillReceiveProps is a special case in that it specifically blocks default setState behaviour and thus should prevent a re-render in queueStateChanges. It does this by setting component._blockRender to be true until after componentWillReceiveProps has finished. setState should only change the current component.state object, I'm not sure how instance._lastInput is coming into this?

@Havunen
Copy link
Member

Havunen commented Mar 18, 2017

It would truly help if you guys @kanzelm3 @OMLET could setup minimal steps to reproduce. Maybe new repository?

@kanzelm3
Copy link
Contributor Author

I will work on it tonight, I had to take a break, my daughter's birthday party was today. You are correct that blockRender is set on the component that componentWillReceiveProps is called on, the issue is that the action that's dispatched triggers setState on a parent component that does not have blockRender set, and the whole flow is synchronous.

@trueadm
Copy link
Member

trueadm commented Mar 18, 2017

@kanzelm3 Ah, I see. Maybe we should introduce a sort of global blockRender effect and put it on Inferno.options like we've done with others. This should force all components to behave this way? @longlho @Havunen if you get a chance to try this one, it might be a simple fix to this, it may not though.

@kanzelm3
Copy link
Contributor Author

Yeah that's what I was thinking, I just wasn't sure how feasible adding a global blockRender would be and wasn't sure where to start.

@kanzelm3
Copy link
Contributor Author

Because if blockRender was set globally then any actions dispatched from componentWillReceiveProps that result in parent components calling setState would then be queued instead of updating synchronously.

@trueadm
Copy link
Member

trueadm commented Mar 18, 2017

I'd add a blockRender to https://github.com/infernojs/inferno/blob/master/packages/inferno/src/core/options.ts:

Remove it as a property _blockRender from the component and instead check from options in the current places it does _blockRender checks from the component. Can you let me know if this fixes your problem?

This doesn't really have anything to do with sync vs async. In fact, in many ways this is an unintended side-effect that React should never have supported and in the future, with Fiber will likely go away in the form of a new API.

@Havunen
Copy link
Member

Havunen commented Mar 21, 2017

@trueadm I dont think that will work... Wouldn't that mean all renders are blocked accross the board while one component got props?

@Havunen
Copy link
Member

Havunen commented Mar 21, 2017

Btw we have faced same issue as described here in our company. Its also related to componentWillReceiveProps => do setState => raise callback => do setState there in parent component, and fail.

@Havunen
Copy link
Member

Havunen commented Mar 21, 2017

I have verified this is not related to componentWillReceiveProps only but also componentWillUpdate (most likely all lifecycle events)

@Havunen
Copy link
Member

Havunen commented Mar 21, 2017

I pushed few variations of test cases to dev to reproduce this issue.

@Havunen
Copy link
Member

Havunen commented Mar 25, 2017

Good news this will be fixed in 1.5

  • Using setStateSync will still fail due to its synchronous nature.
  • setState will start going async now correctly and this issue wont happen

@kanzelm3
Copy link
Contributor Author

@Havunen that's great news, looking forward to it and sorry I couldn't help more.

@Havunen
Copy link
Member

Havunen commented Mar 27, 2017

Closing as 1.5 has been released! 👍

@Havunen Havunen closed this as completed Mar 27, 2017
@kirill-kruchkov
Copy link

Hello. The issue seems to be back again starting v1.6 (also reproduced in 3.0.6) with error message "Uncaught TypeError: Cannot read property 'flags' of null" when dispatching redux actions from componentWillReceiveProps.

@nixedmelon
Copy link

I'm getting the same problem as @kirill-kruchkov. Will look into...

@mienaikoe
Copy link

mienaikoe commented Jul 18, 2017

Also getting this problem. Seems to happen only when navigating from route to route for me. Happens even when I use the back button. I have an onEnter method for each of my routes. idk if that would do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

7 participants