Skip to content

Deprecate isMounted #5465

Closed
Closed
@jimfb

Description

@jimfb
Contributor

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:

Activity

quantizor

quantizor commented on Nov 16, 2015

@quantizor
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

jimfb commented on Nov 16, 2015

@jimfb
ContributorAuthor

@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

quantizor commented on Nov 16, 2015

@quantizor
Contributor

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

nvartolomei

nvartolomei commented on Nov 16, 2015

@nvartolomei

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

quantizor commented on Nov 16, 2015

@quantizor
Contributor

@nvartolomei Look at the ES6 promise spec.

zpao

zpao commented on Nov 16, 2015

@zpao
Member

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

jimfb commented on Nov 16, 2015

@jimfb
ContributorAuthor

@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

quantizor commented on Nov 16, 2015

@quantizor
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

jimbolla commented on Nov 16, 2015

@jimbolla

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

istarkov commented on Nov 18, 2015

@istarkov
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

quantizor commented on Nov 18, 2015

@quantizor
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

ir-fuel commented on Nov 20, 2015

@ir-fuel

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.

66 remaining items

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @mo@zpao@dantman@pygy@koistya

        Issue actions

          Deprecate `isMounted` · Issue #5465 · facebook/react