-
Notifications
You must be signed in to change notification settings - Fork 48.5k
Docs - Recommendations for isMounted() alternative #3417
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
Comments
I think the recommended way is to make sure you're properly cleaning up resources and canceling timers, etc. in componentWillUnmount, so that you don't need to worry later about whether the component is still mounted. @sebmarkbage can probably say more. |
The common case where this is difficult is for Promises (terrible design to not make them cancelable) but we'll probably add some custom workaround for Promises, for everything else, we recommend that you clean up and timers during |
I want to guard async |
I'd love to see a solution for the Promises issue. We use Promises in our app's database layer, so some of our React components do things (roughly) like this:
We could certainly create a Flux store, giving us a synchronous One solution that would be interesting would be to use Promise chaining:
Another solution we've thought about is wrapping our Promises in a tiny class that makes them "subscribable". Very much defeats the point of Promise syntax, but we could say:
|
+1 to @franleplant question. |
I'm experimenting with some code to handle the uncancellable promises issue. My (not thoughly tested) solution is to wrap the callback, like so:
Notice the callback in
Basically, it keeps a bag of pending callbacks, which is nullified on Disclaimer: I just whipped this up and haven't thoroughly tested it yet. Feel free to use it, break it, and tell me why it doesn't work. |
My current hack for this: /* ... */
componentDidMount() {
this.mounted = true;
}
componentWillUnmount() {
this.mounted = false;
}
/* ... */ Forgive me for my sins but it works |
Similar to @glittershark but I prefer using state, found this useful when using class ... extends Component {
constructor(props){
super(props);
this.state = { mounted: false };
}
componentDidMount(){
this.setState({ mounted: true });
}
render(){
var { mounted } = this.state;
if(mounted){
...
}
...
}
} |
An Implementing this in userland seems like the best choice - then we could benefit from Promise libraries like Bluebird that actually do implement cancelable promises. |
I'm currently using a similar approach to @jimbolla where the fulfillment handler is wrapped in a proxy function which passes its arguments onto the original callback unless it is canceled, in which case the proxy becomes a no-op. The proxy is canceled on unmount: https://gist.github.com/robertknight/857d263dd4a68206da79 |
I ended up using @glittershark 's approach, too. My state was not being updated. Here is the code I had:
And here's what I changed it to:
|
@hectron I see a few issues with this approach:
class Foo extends React.Component {
componentDidMount() {
window.addEventListener('mousedown', this.pageClick, false);
}
componentWillUnmount() {
window.removeEventListener('mousedown', this.pageClick, false);
}
// Use a combination of a property initializer and an arrow function to bind!
// Compiles to, roughly:
// `_this = this; this.pageClick = function(e) { if (_this.shouldDisableToggle ...`
pageClick = (e) => {
if (this.shouldDisableToggle(e.target)) { /* ... */ }
};
} Otherwise, in the constructor: constructor(props, context) {
super(props, context);
this.pageClick = this.pageClick.bind(this);
} |
Tracking the cancellable promise seems fairly verbose, and it forces adding some state to the component which seems undesirable (keeping a reference to the promise so you can cancel it in It would be nice if the logic could be self-contained similar to cancellable observables: componentDidMount() {
makeCancelable( doAsyncThing() )
.takeUntil(this.componentUnmounted)
.then (results) => {
this.setState(results)
}
} But I'm not sure how React would generate the unmounted signal for that to work. |
See facebook/react#3417 for more details.
See facebook/react#3417 for more details.
AcceptDialog was using `isMounted` which was causing a deprecation warning (see: facebook/react#3417) It was also using `createReactClass`. This commit updates it in both ways.
The docs at http://facebook.github.io/react/docs/component-api.html give a use case for
isMounted()
and mention that it is not available for ES6 classes, but don't explicitly call it out as deprecated.It would be useful if they clarified what the recommended alternative is.
Most of the uses I have in my existing code are for dealing with the case where a component gets unmounted before a timeout setup for animation etc. after the component is mounted expires.
The text was updated successfully, but these errors were encountered: