Closed adriansdev closed 8 years ago
(TL;DR => see the hot fix at the end)
I have a similar issue that the onChange
will be called after the calling the unlisten
in componentWillUnmount
.
After a little investigation, I suspect the problem is due to the here.
unlisten
function will put the onChange
in an array called toUnsubcribe
which is storing the callbacks to be unsubscribed.alt.js
, it will loop through all the subscriptions (callbacks). If parent's state is changed and causes any child component to be unmounted (which invokes the unlisten here), the subscription of the child component will still be called which will lead to this issue.var Parent = React.createClass({
onChange: function() {
// change the state to invoke the unmounting of child
this.setState({showChild: false})
},
render: function() {
return <div>Parent: {this.state.showChild? <Child /> : ""}</div>;
}
});
var Child = React.createClass({
componentWillUnMount: function() {
// invoked by parent's changing state
Store.unlisten(this.onChange)
},
onChange: function() {
// still be invoked because this callback still exists in the loop of subscrptions
},
render: function() {
return <div>Child</div>;
}
});
Add following before line#1358 of alt.js
(Actually, I need to add the following before the line#30 of transmitter.js
in {project_dir}/npm_modules/transmitter/dist/transmitter.js
in order to make the webpack working...)
if (toUnsubscribe.indexOf(subscription) > -1) return;
What version of Alt are you all using? How are you setting up the subscription to the store?
Mine is 0.18.4
. I call the listen
in componentDidMount
.
can you give a failing test case? something which ideally does not involve React, just Alt.
Hi. same for me, 0.18.4, and ES6 style binding. The hotfix seems to work.
I can't reproduce with the given code, which I've modified it to:
const React = require('react')
const Alt = require('alt')
const ReactDOM = require('react-dom')
const alt = new Alt()
const Store = alt.createStore(function () {
this.bindAction('noop', () => console.log('Store hit'))
}, 'teststore')
var counter = 0
// const transmitter = require('transmitter')
//
// const bus = transmitter()
//
// const sub = bus.subscribe((x) => {
// console.log('Data', x)
// sub.dispose()
// })
//
// bus.push(123)
//
// bus.push(456)
var Parent = React.createClass({
getInitialState: function () {
return { showChild: true }
},
onChange: function() {
// change the state to invoke the unmounting of child
this.setState({showChild: false})
alt.dispatch('noop', ++counter)
},
render: function() {
return React.createElement('div', {
onClick: this.onChange,
}, 'Parent', this.state.showChild ? React.createElement(Child) : '')
}
});
var x = null
var Child = React.createClass({
componentWillMount: function() {
x = Store.listen(this.onChange)
},
componentWillUnmount: function() {
console.log('unmounting...')
// console.log(x)
// invoked by parent's changing state
Store.unlisten(this.onChange)
},
onChange: function() {
console.log('Hi')
// still be invoked because this callback still exists in the loop of subscrptions
},
render: function() {
return React.createElement('div', null, 'Child')
}
});
ReactDOM.render(React.createElement(Parent), document.getElementById('app'))
but there's no issue present. I also wrote a transmitter only failing case but that's working fine too.
I'm using latest alt and react. Feel free to post a test case and I can reopen.
I think it occurs when a parent view unloads a child view and both listen to same store. The child unmounts and unlistens but the state change still gets triggered, causing an error. Can pass store properties down to avoid but neater to reference store locally
@adriansprod agree.
The sample code above demonstrates the scenario you're describing but the bug is not happening. My guess is you're not unsubscribing properly?
I have tried a codepen with variation of your code but can't replicate. unsubscribing is normal
Am mainly using React Native and ES6 so possibly makes a difference. Will investigate more when have chance
I'd look into unlisten
, the function that you pass into unlisten must be the same exact function that was passed into listen. This means you can't bind it on the spot. I know, this is a bad API...I'd recommend just using the unsubscribe function that is returned when calling .listen
.
@goatslacker From your code above, can you add an Action
to update the Store
and let it triggers onChange
?
It seems that the bug would happen when Store is looping through the subscriptions.
onChange
of both parent and child would be triggeredonChange
is called and its state is changed ---> unmount the childcomponentWillUnmount
---> unlisten
is calledonChange
onChange
is called ---> by changing the state of child here, the error would be triggered because this child should be unmounted alreadyUpdated the code base on your suggestions:
const React = require('react')
const Alt = require('alt')
const ReactDOM = require('react-dom')
const alt = new Alt()
const Store = alt.createStore(function () {
this.bindAction('noop', () => console.log('Store hit'))
}, 'teststore')
var counter = 0
var Parent = React.createClass({
getInitialState: function () {
return { showChild: true }
},
componentWillMount: function() {
Store.listen(this.onChange)
},
componentWillUnmount: function() {
// invoked by parent's changing state
Store.unlisten(this.onChange)
},
onClickHandler: function() {
alt.dispatch('noop', ++counter)
},
onChange: function() {
// change the state to invoke the unmounting of child
this.setState({showChild: false})
},
render: function() {
return React.createElement('div', {
onClick: this.onClickHandler,
}, 'Parent', this.state.showChild ? React.createElement(Child) : '')
}
});
var Child = React.createClass({
componentWillMount: function() {
Store.listen(this.onChange)
},
componentWillUnmount: function() {
// invoked by parent's changing state
Store.unlisten(this.onChange)
},
onChange: function() {
console.log('Hi, I should only be displayed once. Keep clicking on "Parent".')
// still be invoked because this callback still exists in the loop of subscrptions
},
render: function() {
return React.createElement('div', null, 'Child')
}
});
ReactDOM.render(React.createElement(Parent), document.getElementById('app'))
@goatslacker Thanks for the edit. I modified the code to look like what I have right now. You can have a look here: https://jsfiddle.net/69z2wepo/49142/
First click on parent, then click on children. After children is gone, you should see the error in the console.
Now I am wondering if this is due to my bad coding style.......
thats the problem @peteroid. good to have an example
Thanks for the bug report. I'll get a fix out :)
alt@0.18.5
bf9ee81c9feb86092fd2d779a18d40790ac08e84
Great, thanks for the update
Thanks for the fix! Appreciate it.
Thank you both for reporting and for the repro case :)
Unlistening an Alt store doesn't seem reliable now and state changes still trigger the listener and cause errors after a view is unmounted. There was a long discussion on 27 May on https://gitter.im/goatslacker/alt with the same issue but no answer.