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

Don't consider forceUpdates after unmounting an error? #1247

Closed
markijbema opened this Issue Mar 12, 2014 · 12 comments

Comments

Projects
None yet
8 participants
@markijbema
Contributor

markijbema commented Mar 12, 2014

I came across an issue with React.Backbone, which in my opinion is an issue with react (though it is possible to work around it, but by implementing what imho should be in React).

Let's say i have a backbone model (or some other event source), I listen on for changes. But changes tend to come together (adds on collections for instance), and I don't want to rerender, because I know it's very probable a new change will arrive. So I take the naive code, and add a debounce or a throttle (see for some background clayallsopp/react.backbone#9 ).

When I unmount the component, I of course unbind the event listeners. However, there is no easy way to remove the throttlled operations still in the air. So now the debounce comes to the end of it's time out, and triggers the throttled forceUpdate.

I wonder why React would consider a forceUpdate after unmount an exception/error. I think a warning might be warranted, but isn't a default behaviour of throwing away the call perfectly fine? Otherwise you're demanding of the users of components to either wrap forceUpdate in a safeForceUpdate which checks isMounted, or all bound events must be unbound synchronous (which I would guess isn't always possible).

If indeed everyone agrees I would be willing to make a PR, but it sounds like something which not everyone might agree on, so I'd be interested in background/opinions.

@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Mar 12, 2014

Contributor

forceUpdate is not really a recommended (general) approach to updating components, what stops you from using setState? Or are you perhaps always reading your data directly from Backbone?

Contributor

syranide commented Mar 12, 2014

forceUpdate is not really a recommended (general) approach to updating components, what stops you from using setState? Or are you perhaps always reading your data directly from Backbone?

@markijbema

This comment has been minimized.

Show comment
Hide comment
@markijbema

markijbema Mar 12, 2014

Contributor

Yes, I'm reading directly from backbone. So the render uses this.props.model.get('username'), and if username changes in model. Of course I could use setState, and do something like setState(backboneReactStep:, this.state+1), and just not use this state. But it seems that this is exactly what forceUpdate is for, or am I misinterpreting what it's for. I understood that it's ok to call forceUpdate to indicate that this component is dirty, in the sense that it's state outside of this.state has changed.

I understand that there are other ways to use react (and I'm considering using immutable collections, but it's not obvious how we should transfer there from backbone models). However, backbone with react still seems to be an advisable choice (in the sense that it seems to be the defacto react-beginners advice), so I think it should be supported.

I'm especially interested in what the advantage is of considering it an error. I can see how it can indicate an issue, but I don't see how it ever really would be a problem.

Contributor

markijbema commented Mar 12, 2014

Yes, I'm reading directly from backbone. So the render uses this.props.model.get('username'), and if username changes in model. Of course I could use setState, and do something like setState(backboneReactStep:, this.state+1), and just not use this state. But it seems that this is exactly what forceUpdate is for, or am I misinterpreting what it's for. I understood that it's ok to call forceUpdate to indicate that this component is dirty, in the sense that it's state outside of this.state has changed.

I understand that there are other ways to use react (and I'm considering using immutable collections, but it's not obvious how we should transfer there from backbone models). However, backbone with react still seems to be an advisable choice (in the sense that it seems to be the defacto react-beginners advice), so I think it should be supported.

I'm especially interested in what the advantage is of considering it an error. I can see how it can indicate an issue, but I don't see how it ever really would be a problem.

@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Mar 12, 2014

Contributor

Ah, that might make sense yes, I'm not really familiar with Backbone so can't say if there's a better way of doing it, but ultimately you'd want to pass relevant data as props, but perhaps that's not an option.

From the viewpoint of forceUpdate() === setState(...) what you say makes sense. From a naming perspective though the current behavior seems correct to me, forceUpdate() on an unmounted component means that you're telling it to update, but "it" doesn't exist. setState() on a mounting component simply means that there's nothing visual to update, but being able to update the state still makes sense to me, as in componentWillMount. That's not to say that what you're suggesting is wrong, but the current behavior seems in line with the naming convention at least.

Also, you could consider using a mixin, it could define this.modelUpdated() or whatever makes sense, avoiding weird globals.

(So this is for someone more involved in the project to decide on ofc)

Contributor

syranide commented Mar 12, 2014

Ah, that might make sense yes, I'm not really familiar with Backbone so can't say if there's a better way of doing it, but ultimately you'd want to pass relevant data as props, but perhaps that's not an option.

From the viewpoint of forceUpdate() === setState(...) what you say makes sense. From a naming perspective though the current behavior seems correct to me, forceUpdate() on an unmounted component means that you're telling it to update, but "it" doesn't exist. setState() on a mounting component simply means that there's nothing visual to update, but being able to update the state still makes sense to me, as in componentWillMount. That's not to say that what you're suggesting is wrong, but the current behavior seems in line with the naming convention at least.

Also, you could consider using a mixin, it could define this.modelUpdated() or whatever makes sense, avoiding weird globals.

(So this is for someone more involved in the project to decide on ofc)

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Mar 14, 2014

Member

Doesn't setState throw too on an unmounted instance?

Member

sophiebits commented Mar 14, 2014

Doesn't setState throw too on an unmounted instance?

@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Mar 14, 2014

Contributor

@spicyj Yeah you're right, I distinctly remember that not being the case, but apparently it is. However, forceUpdate cannot be called during mounting while setState can.

Contributor

syranide commented Mar 14, 2014

@spicyj Yeah you're right, I distinctly remember that not being the case, but apparently it is. However, forceUpdate cannot be called during mounting while setState can.

@markijbema

This comment has been minimized.

Show comment
Hide comment
@markijbema

markijbema Mar 14, 2014

Contributor

I can see why setting something before mounting can always be considered an error. I think after unmounting should be considered a warning though (though I can imagine that in implementing it might be easier to consider both as warning)

Contributor

markijbema commented Mar 14, 2014

I can see why setting something before mounting can always be considered an error. I think after unmounting should be considered a warning though (though I can imagine that in implementing it might be easier to consider both as warning)

@mrjoes

This comment has been minimized.

Show comment
Hide comment
@mrjoes

mrjoes Apr 4, 2014

I'm having similar problem. I make AJAX call and if result comes back after component was unmounted, I'm getting invariant error, which breaks React app completely - it no longer can mount other components, etc.

Wrapping all response handlers with if (this.isMounted()) is an option, but a bit cumbersome.

mrjoes commented Apr 4, 2014

I'm having similar problem. I make AJAX call and if result comes back after component was unmounted, I'm getting invariant error, which breaks React app completely - it no longer can mount other components, etc.

Wrapping all response handlers with if (this.isMounted()) is an option, but a bit cumbersome.

@ustun

This comment has been minimized.

Show comment
Hide comment
@ustun

ustun Apr 5, 2014

@mrjoes One other way could be to push all ajax requests handlers to an array, and go through that array in componentWillUnmount to abort all of them. Of course, you'll need to remove them from the array on complete of the ajax call. http://stackoverflow.com/questions/446594/abort-ajax-requests-using-jquery

ustun commented Apr 5, 2014

@mrjoes One other way could be to push all ajax requests handlers to an array, and go through that array in componentWillUnmount to abort all of them. Of course, you'll need to remove them from the array on complete of the ajax call. http://stackoverflow.com/questions/446594/abort-ajax-requests-using-jquery

@mrjoes

This comment has been minimized.

Show comment
Hide comment
@mrjoes

mrjoes Apr 5, 2014

@ustun This won't work for me. Data access layer is separated from UI and returns a promise. I don't have access to underlying AJAX request object, so I can't cancel it. Cancelling all requests is not an option.

mrjoes commented Apr 5, 2014

@ustun This won't work for me. Data access layer is separated from UI and returns a promise. I don't have access to underlying AJAX request object, so I can't cancel it. Cancelling all requests is not an option.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Apr 5, 2014

Member

This is exactly the reason I left this in. Having a data layer that accumulates potentially long running request is death by a thousand cuts. You need a way to handle canceling.

Many data layers set up infinite subscriptions for data updates. Not having a clean up mechanism is clearly a memory leak which can bring the entire React subtree with it.

Streams or Observables are good primitives that have cleanup semantics. I consider a purely Promise based data layer inherently flawed for this reason. If you want the output to always be promise based, you can pass some kind of handler down your data layer with a cancelability hook.

Ultimately I think the solution is Stream/Observable based. As soon as we have a proposal for it, we would build in native support for cancelation but for now you can do it in a mixin.

Member

sebmarkbage commented Apr 5, 2014

This is exactly the reason I left this in. Having a data layer that accumulates potentially long running request is death by a thousand cuts. You need a way to handle canceling.

Many data layers set up infinite subscriptions for data updates. Not having a clean up mechanism is clearly a memory leak which can bring the entire React subtree with it.

Streams or Observables are good primitives that have cleanup semantics. I consider a purely Promise based data layer inherently flawed for this reason. If you want the output to always be promise based, you can pass some kind of handler down your data layer with a cancelability hook.

Ultimately I think the solution is Stream/Observable based. As soon as we have a proposal for it, we would build in native support for cancelation but for now you can do it in a mixin.

@mrjoes

This comment has been minimized.

Show comment
Hide comment
@mrjoes

mrjoes Apr 6, 2014

Well, in my case DAL handles timeout, so caller is promised with either response or an error.

My problem is with React handling of setState() in unmounted component. I'm OK with a warning in debug mode, but I'm not sure it should fail with an error.

mrjoes commented Apr 6, 2014

Well, in my case DAL handles timeout, so caller is promised with either response or an error.

My problem is with React handling of setState() in unmounted component. I'm OK with a warning in debug mode, but I'm not sure it should fail with an error.

sophiebits referenced this issue in Khan/react-components Jun 29, 2014

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Mar 18, 2016

Member

The invariant has been replaced with a warning in #4091.
Still, it’s best to avoid the warning. To do that, you can:

  1. When possible, use a cancelable async primitive (e.g. an Observable) so you can clean up on unmounting.
  2. If for some reason you can’t do this, set a flag (e.g. this.unmounted = true) in your componentWillUnmount and check for that flag in the callback. It’s not pretty but it makes the problem with the lack of cancellation more apparent and visible.

I’m closing but please let me know if there is something I missed.

Member

gaearon commented Mar 18, 2016

The invariant has been replaced with a warning in #4091.
Still, it’s best to avoid the warning. To do that, you can:

  1. When possible, use a cancelable async primitive (e.g. an Observable) so you can clean up on unmounting.
  2. If for some reason you can’t do this, set a flag (e.g. this.unmounted = true) in your componentWillUnmount and check for that flag in the callback. It’s not pretty but it makes the problem with the lack of cancellation more apparent and visible.

I’m closing but please let me know if there is something I missed.

@gaearon gaearon closed this Mar 18, 2016

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