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

Deprecate isMounted #5465

Closed
jimfb opened this issue Nov 13, 2015 · 50 comments
Closed

Deprecate isMounted #5465

jimfb opened this issue Nov 13, 2015 · 50 comments

Comments

@jimfb
Copy link
Contributor

jimfb commented Nov 13, 2015

isMounted is already unavailable on ES6 classes, and we already have a warning saying we "might" remove them, but we don't actually have a github issue to deprecate them. As per our discussions today, we are basically agreed that we're going to start moving away from isMounted and deprecate it. We still need to figure out some good stories around promises (and related use cases).

This issue is to track progress toward that goal.

For background, please read:

@quantizor
Copy link
Contributor

I don't agree with this. ES6 promises in particular cannot be reliably cancelled on componentWillUnmount, so removing the only way to check if the component is mounted before setState or another action is opening the way for a lot of hard to trace async bugs.

@jimfb
Copy link
Contributor Author

jimfb commented Nov 16, 2015

@yaycmyk Thus the line:

We still need to figure out some good stories around promises (and related use cases).

Please read the background issues I listed, in particular: #2787 (comment)

@quantizor
Copy link
Contributor

I did read the comments. I just find the issues intractable.

@nvartolomei
Copy link

Why promises cant be reliable cancelled? Any sources/proofs/examples?

On Monday, November 16, 2015, Evan Jacobs notifications@github.com wrote:

I don't agree with this. ES6 promises in particular cannot be reliably
cancelled on componentWillUnmount, so removing the only way to check if
the component is mounted before setState or another action is opening the
way for a lot of hard to trace async bugs.

@quantizor
Copy link
Contributor

@nvartolomei Look at the ES6 promise spec.

@zpao
Copy link
Member

zpao commented Nov 16, 2015

This is a longer term goal, not something that is happening immediately. But we want to track the planning and discussions in a single place and not across comments in every issue when this comes up. We are aware of the problem of Promises currently being uncancellable which is a major reason we haven't already done this.

@jimfb
Copy link
Contributor Author

jimfb commented Nov 16, 2015

@yaycmyk To over-simplify a very complex issue... the comments are saying... using isMounted to avoid setState for unmounted components doesn't actually solve the problem that the setState warning was trying to indicate - in fact, it just hides the problem. Also, calling setState as the result of a promise is a bit of an anti-pattern anyway, since it can cause race conditions which won't necessarily show up in testing. Thus we want to get rid of it, and figure out a "best practice" recommendation for using promises with React.

I agree the issues are a bit inscrutable, but that's largely because it's a complex issue that we're still figuring out and don't yet have a canned response for.

@quantizor
Copy link
Contributor

calling setState as the result of a promise is a bit of an anti-pattern anyway, since it can cause race conditions which won't necessarily show up in testing

We can agree to disagree on that one. There are times when content is being fetched asynchronously and you don't want to have to go through a full-scale rerender to pop that content in once it is resolved. I use it specifically in an infinite table view implementation where a full virtual rerender would be unnecessary.

@jimbolla
Copy link

You might not be able to cancel a promise, but you can make it dereference the component on unmount, like so:

const SomeComponent = React.createClass({
    componentDidMount() {
        this.protect = protectFromUnmount();

        ajax(/* */).then(
            this.protect( // <-- barrier between the promise and the component
                response => {this.setState({thing: response.thing});}
            )
        );
    },
    componentWillUnmount() {
        this.protect.unmount();
    },
});

The important distinction is when this.protect.unmount() is called in componentWillUnmount, all callbacks get dereferenced, meaning the component gets dereferenced, and then when the promise completes, it just calls a no-op. This should prevent any memory leaks related to promises references unmounted components. source for protectFromUnmount

@istarkov
Copy link
Contributor

This simple method can be used to add cancel to any promise

const makeCancelable = (promise) => {
  let hasCanceled_ = false;

  const wrappedPromise = new Promise((resolve, reject) => {
    promise.then((val) =>
      hasCanceled_ ? reject({isCanceled: true}) : resolve(val)
    );
    promise.catch((error) =>
      hasCanceled_ ? reject({isCanceled: true}) : reject(error)
    );
  });

  return {
    promise: wrappedPromise,
    cancel() {
      hasCanceled_ = true;
    },
  };
};

EDIT: Updated for correctness/completeness.

HOW TO USE

const somePromise = new Promise(r => setTimeout(r, 1000));

const cancelable = makeCancelable(somePromise);

cancelable
  .promise
  .then(() => console.log('resolved'))
  .catch(({isCanceled, ...error}) => console.log('isCanceled', isCanceled));

// Cancel promise
cancelable.cancel();

@quantizor
Copy link
Contributor

Listing ways to sup-up ES6 promises to make them cancellable is besides the point. The intent should be to provide a solution that works WITH the spec rather than trying to work AROUND the spec.

@ir-fuel
Copy link

ir-fuel commented Nov 20, 2015

I agree. Instead of simply checking if the component is still mounted when we receive the promise result we have to resort to all kinds of magic so we can "unbind" our promise from the component it's supposed to set its result in, clearly fighting against the way promises are designed.
To me it feels like overengineering a solution where a simple test is the easiest way to take care of this.

@lasekio
Copy link
Contributor

lasekio commented Nov 20, 2015

We can keep simple checking just by:

React.createClass(function() {
  componentDidMount: function() {
    this._isMounted = true;

    ajax(/* */).then(this.handleResponse);
  }

  handleResponse: function(response) {
    if (!this._isMounted) return; // Protection

    /* */
  }

  componentWillUnmount: function() {
    this._isMounted = false;
  }
});

@ir-fuel
Copy link

ir-fuel commented Nov 20, 2015

This is of course my opinion, but it seems to me that async data loading with a promise inside a react component is such a common scenario that it should be covered by react, instead of having to write our own boilerplate code.

@lasekio
Copy link
Contributor

lasekio commented Nov 20, 2015

The problem is, that to fallow the true mount state we must add listener when react will finish DOM mount process in each component (the same, which attach componentDidMount, if defined), but it will affect on perf, because we don't need to fallow it everywhere. Component dont listen DOM mount ready by default since componentDidMount is undefined.

@RoyalIcing
Copy link

What if setState could be passed a chained promise which resolves to the desired state changes? If the component unmounts, then if there are any pending promises their eventual result is ignored.

@koistya
Copy link
Contributor

koistya commented Dec 17, 2015

@istarkov nice pattern, like it! Here is slightly altered API for it:

// create a new promise
const [response, cancel] = await cancelable(fetch('/api/data'));

// cancel it
cancel();

@dtertman
Copy link

Since I'm new to React and reading docs, just to throw this out there : the Load Initial Data via Ajax tip uses .isMounted(), so the website disagrees with the website. It would be great to see a complete Tip about how to cancel the initial load in componentWillUnmount, maybe using @istarkov's pattern above.

@jimfb
Copy link
Contributor Author

jimfb commented Jan 19, 2016

@dtertman Fixed in #5870, will be online when the docs get cherry-picked over.

@dtertman
Copy link

@jimfb thanks, not sure how I missed that in search.

@vpontis
Copy link

vpontis commented Mar 1, 2016

@istarkov not sure if this was intentional but your makeCancelable does not handle if the original promise fails. When the original promise is rejected, no handler gets called.

This does not seem ideal because you may still want to handle an error on the original promise.

Here is my proposal for a makeCancelable that handles a rejection in the original promise:

const makeCancelable = (promise) => {
  let hasCanceled_ = false;

  const wrappedPromise = new Promise((resolve, reject) => {
    promise.then((val) =>
      hasCanceled_ ? reject({isCanceled: true}) : resolve(val)
    );
    promise.catch((error) =>
      hasCanceled_ ? reject({isCanceled: true}) : reject(error)
    );
  });

  return {
    promise: wrappedPromise,
    cancel() {
      hasCanceled_ = true;
    },
  };
};

I'm not sure where I stand on if making cancelable promises is a good idea, but if we are going to make promises cancelable, we should preserve the underlying behavior :).

@istarkov
Copy link
Contributor

istarkov commented Mar 1, 2016

@vpontis 👍

@vpontis
Copy link

vpontis commented Mar 1, 2016

@istarkov your original post is referenced here: https://facebook.github.io/react/blog/2015/12/16/ismounted-antipattern.html

Want to update your post or should I message the author of the post?

@jimfb
Copy link
Contributor Author

jimfb commented Mar 1, 2016

@vpontis Thanks, I'll fix! (#6152)

@adeelibr
Copy link

adeelibr commented Jul 5, 2018

It's 2018 is there an even better approach then the one's mentioned above?

@npblubb
Copy link

npblubb commented Jul 5, 2018

yes you can use some navigation frameworks that have a documentation twice the size of react native but is very professionel

@dantman
Copy link
Contributor

dantman commented Feb 5, 2019

These snippets for "canceling" a promise aren't that great IMHO. The cancelled promises will still not resolve until the original promise resolves. So memory cleanup won't happen till it would if you just used an isMounted trick.

A proper cancelable promise wrapper would have to make use of a second promise and Promise.race. i.e. Promise.race([originalPromise, cancelationPromise])

@benmmurphy
Copy link

@benmmurphy's solution is closer but nulls out the wrong variables so the promise handlers are not dereferenced.

I think my solution works but I don't know enough about what promises the javascript runtime gives to know for sure. If you run the solution under node in your test harness it correctly GCs the value. My solution assigned the the resolve/reject functions to a higher scope and then nulled these values out when cancel was called. However, the functions were still available in the lower scope but not referenced. I think modern javascript engines don't capture variables in a closure unless they are referenced. I think this used to be a big problem where people would accidentally create DOM leaks because they did stuff like: var element = findDOM(); element.addEventListener('click', function() {}); and element would be referenced in the closure even though it wasn't used in the closure.

@drprasad-capillary
Copy link

@hjylewis @benmmurphy why do we need to dereference handlers ?? after handlers excuted, garbage collection any way happens, right ??

@benmmurphy
Copy link

These snippets for "canceling" a promise aren't that great IMHO. The cancelled promises will still not resolve until the original promise resolves. So memory cleanup won't happen till it would if you just used an isMounted trick.

A proper cancelable promise wrapper would have to make use of a second promise and Promise.race. i.e. Promise.race([originalPromise, cancelationPromise])

@hjylewis and mine do you actually work you can verify it with node weak. but looking at them again i agree neither of them are idiosyncratic written promise code. as a promise user you would probable expect a 'cancelled' promise to resolve in the rejected state and neither of them do this. though, possibly in the case of a component this is a solution that would be easier to use because you don't have to write extra code to ignore the reject handler.

i think an idiosyncratic rejectable promise would use Promise.race([]) to build a cancellable promise. it works because when a promise becomes resolved the pending callbacks are deleted so at the point there would be no reference chain from the browser network to your component because there would be no longer a reference between the race promise and the component.

@ketysek
Copy link

ketysek commented Mar 1, 2019

I'm curious if it's somehow possible to use Promise.all() with those cancelable promises and avoid uncaught errors in browsers console ... because I'm able to catch only first cancellation error, others remains uncaught.

@adeelibr
Copy link

It's 2018 is there an even better approach then the one's mentioned above?

Any better approach to cancel a promise execution i.e, setTimeout, API Calls etc.. It's 2019 😭 😞

@adeelibr
Copy link

There is Promise cancellation thread going on TC39, (I think) it's of relevance here (maybe .. not sure)
tc39/proposal-cancellation#24

@paul4156
Copy link

Any better approach to cancel a promise execution i.e, setTimeout, API Calls etc.. It's 2019 😭 😞

Are we looking for something like

const promise = new Promise(r => setTimeout(r, 1000))
  .then(() => console.log('resolved'))
  .catch(()=> console.log('error'))
  .canceled(() => console.log('canceled'));

// Cancel promise
promise.cancel();

filipemir added a commit to filipemir/reactjs.org that referenced this issue Nov 12, 2019
As discussed in [this thread](facebook/react#5465 (comment)) and expanded [here](https://github.com/hjylewis/trashable/blob/master/PROOF.md), the solution proposed for cancelling promises when a component unmounts doesn't actually address the possible memory leaks. Interestingly, I don't believe @hjylewis's approach does so fully either (see [this issue](hjylewis/trashable#6)) but it does so in the vast majority of cases. If I'm following the conversation along correctly, I think the docs should be updated to reflect the better answer. And if it's not the better answer, I would love to hear the arguments against it.
@impulsgraw

This comment has been minimized.

@baldwinjj
Copy link

I know this is an old thread but I figure I'd chime in just in case anyone else comes across it looking for a generic solution to cancel promises as I did. @dantman is right, @istarkov's solution doesn't behave as I was expecting because the canceled promise doesn't reject until after the wrapped promise has resolved. Here is a solution that better matches my (and perhaps others') expectations. It immediately rejects whenever cancel is called.

const makeCancelable = (promise) => {
  let onCancel;
  const cancelPromise = new Promise((resolve, reject) => {
    onCancel = () => reject({ isCanceled: true });
  });
  return {
    promise: Promise.race([promise, cancelPromise]),
    cancel: onCancel,
  };
};

Here is an example of both solutions in action: https://stackblitz.com/edit/js-2kfvyt?file=index.js

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

No branches or pull requests