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

How can we fetch data async in reaction to props changes with getDerivedStateFromProps? #26

Closed
felixfbecker opened this Issue Feb 14, 2018 · 29 comments

Comments

Projects
None yet
@felixfbecker

felixfbecker commented Feb 14, 2018

I couldn't find this mentioned in the RFC.
Currently almost all of our components implement compoentWillReceiveProps to trigger some async operation in reaction to props changes, then call setState asynchronously. Simple example:

componentWillReceiveProps(newProps) {
  if (this.props.userID !== newProps.userID) {
    this.setState({ profileOrError: undefined })
    fetchUserProfile(newProps.userID)
      .catch(error => error)
      .then(profileOrError => this.setState({ profileOrError }))
  }
}

I.e. the new derived state from the Props is not derived synchronously, but asynchronously. Given that getDerivedStateFromProps is sync, I don't see how this could work with the new API.

Note that the above example is very simple, is prone to race conditions if the responses arrive out of order and does no cancellation on new values or component unmount.

We usually make use of RxJS Observables to solve all those issues:

private propsUpdates = new Subject<Props>()
private subscription: Subscription

componentDidMount() {
  this.subscription = this.propsUpdates
    .pipe(
      map(props => props.userID),
      distinctUntilChanged(),
      switchMap(userID => fetchUserProfile(userID).pipe(catchError(error => [error])))
    )
    .subscribe(profileOrError => {
      this.setState({ profileOrError })
    })
  this.propsUpdates.next(this.props)
}

componentWillReceiveProps(newProps) {
  this.propsUpdates.next(newProps)
}

componentDidUnmount() {
  this.subscription.unsubscribe()
}

So now that the methods we were making use of are deprecated, I wonder were we doing this wrong all the time? Is there already a better way to do this with the old API? And what is the recommended way to do this with the new API?

@TrySound

This comment has been minimized.

Contributor

TrySound commented Feb 14, 2018

@felixfbecker For side effects you should use componentDidMount, componentDidUpdate hooks. I think the RFC is pretty clear about this.

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Feb 14, 2018

I'm currently in the process of writing some blog posts to go over this, but I'll answer here (with similar content) in case it's helpful for others who come across this issue in the future.

Overview of how React works

I think it would be helpful to start with a quick overview of how React works, and how async rendering will impact class components.

Conceptually, React does work in two phases:

  • The render phase determines what changes need to be made to e.g. the DOM. During this phase, React calls render and then compares the result to the previous render.
  • The commit phase is when React applies any changes. (In the case of React DOM, this is when React inserts, updates, and removes DOM nodes.) React also calls lifecycles like componentDidMount and componentDidUpdate during this phase.

The commit phase is usually very fast, but rendering can be slow. For this reason, async mode will break the rendering work into pieces, pausing and resuming the work to avoid blocking the browser. This means that React may invoke render phase lifecycles more than once before committing, or it may invoke them without committing at all (because of an error or a higher priority interruption).

Render phase lifecycles include the following class component methods:

  • constructor
  • componentWillMount
  • componentWillReceiveProps
  • componentWillUpdate
  • getDerivedStateFromProps
  • shouldComponentUpdate
  • render
  • setState updater functions (the first argument)

Because the above methods might be called more than once, it's important that they do not contain side-effects. Ignoring this rule can lead to a variety of problems. For example, the code snippet you showed above might fetch external data unnecessarily.

Suggested pattern

Given your first example above, I would suggest an approach like this:

class ExampleComponent extends React.Component {
  state = {};

  static getDerivedStateFromProps(nextProps, prevState) {
    // Store prevUserId in state so we can compare when props change.
    // Clear out any previously-loaded user data (so we don't render stale stuff).
    if (nextProps.userId !== prevState.prevUserId) {
      return {
        prevUserId: nextProps.userId,
        profileOrError: null,
      };
    }

    // No state update necessary
    return null;
  }

  componentDidMount() {
    // It's preferable in most cases to wait until after mounting to load data.
    // See below for a bit more context...
    this._loadUserData();
  }

  componentDidUpdate(prevProps, prevState) {
    if (this.state.profileOrError === null) {
      // At this point, we're in the "commit" phase, so it's safe to load the new data.
      this._loadUserData();
    }
  }

  render() {
    if (this.state.profileOrError === null) {
      // Render loading UI
    } else {
      // Render user data
    }
  }

  _loadUserData() {
    // Cancel any in-progress requests
    // Load new data and update profileOrError
  }
}

With regard to the initial data fetching in componentDidMount:

Fetching data in componentWillMount (or the constructor) is problematic for both server rendering (where the external data won’t be used) and the upcoming async rendering mode (where the request might be initiated multiple times). For these reasons, we suggest moving it to componentDidMount.

There is a common misconception that fetching in componentWillMount lets you avoid the first empty rendering state. In practice this was never true because React has always executed render immediately after componentWillMount. If the data is not available by the time componentWillMount fires, the first render will still show a loading state regardless of where you initiate the fetch. This is why moving the fetch to componentDidMount has no perceptible effect in the vast majority of cases.

A couple of other thoughts

I.e. the new derived state from the Props is not derived synchronously, but asynchronously. Given that getDerivedStateFromProps is sync, I don't see how this could work with the new API.

Just in case it's unclear, the old lifecycle, componentWillReceiveProps, was also synchronous. This means that you'd still have a render in between when the new props were set and when external data finished loading. So the suggested pattern above won't require any additional renders.

Note that the above example is very simple, is prone to race conditions if the responses arrive out of order and does no cancellation on new values or component unmount.

Using componentWillReceiveProps exacerbates this problem, for reasons explained above. Hopefully you're using a library like Axios (or managing cancellation in some other way).

So now that the methods we were making use of are deprecated, I wonder were we doing this wrong all the time? Is there already a better way to do this with the old API? And what is the recommended way to do this with the new API?

Legacy lifecycles (like componentWillReceiveProps) will continue to work until React version 17. Even in version 17, it will still be possible to use them, but they will be aliased with an "UNSAFE_" prefix to indicate that they might cause issues. Our preference is that people will migrate to using the new lifecycles though!

@bvaughn bvaughn closed this Feb 14, 2018

@sorahn

This comment has been minimized.

sorahn commented Feb 14, 2018

I was using a similar pattern in lots of my components for ajax requests. The actual change looks hilariously simple on a contrived example, but you get the idea.

https://gist.github.com/sorahn/2cdc344cc698f027a948e3fdf6e0e60f/revisions

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Feb 14, 2018

Yeah, it may be unnecessary to even use the new static lifecycle. It depends on whether you want to immediately clear out stale data (during the next call to render()) or wait until the new data has come in. This is a case-by-case thing, so I showed the more complex example.

@kennethbigler

This comment has been minimized.

kennethbigler commented Apr 19, 2018

@bvaughn did you ever write that article you mentioned you were working on? I would love to read it!

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Apr 19, 2018

@10xjs 10xjs referenced this issue May 17, 2018

Open

Refactor. #34

@qti3e qti3e referenced this issue May 17, 2018

Open

notebook cleanups #21

10 of 10 tasks complete
@eugene-daragan-codex

This comment has been minimized.

eugene-daragan-codex commented May 21, 2018

Shouldn't the componentDidUpdate use current state to determine if the component needs data fetching in the example above? Won't this trigger data fetching twice on initial component rendering - first in componentDidMount and then, when data was fetched and profileOrError was set, in componentDidUpdate method, because it uses prevState for the profileOrError check, which is null (but current state already contains it)?

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented May 21, 2018

Yup! Looks like a typo in componentDidUpdate. Thanks for pointing it out 😄

@migueloller

This comment has been minimized.

migueloller commented Jun 7, 2018

@bvaughn,

Why would one ever want to return null from getDerivedStateFromProps instead of just returning the previous state? I understand that internally, returning null will keep the state the same but returning the previous state will shallowly merge it.

What I'm curious is about the reasoning behind this. Are there any benefits (i.e., maybe performance benefits) to returning null as opposed to always returning an object from getDerivedStateFromProps?

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Jun 7, 2018

Returning null is slightly more performant (since React won't have to merge the state with itself). It also more clearly signals the intent of not updating anything.

@migueloller

This comment has been minimized.

migueloller commented Jun 7, 2018

Gotcha, and I'm assuming this is because React treats the returned object as a state slice so it will always merge it?

Also, I guess if the derived state end up being a complex data structure and a new one is created, it might cause some components to re-render lower in the tree if their shouldComponentUpdate does reference equality.

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Jun 7, 2018

Gotcha, and I'm assuming this is because React treats the returned object as a state slice so it will always merge it?

The returned object is treated as a partial state update, just like an update passed to setState- both get merged into the previous/existing state.

Also, I guess if the derived state end up being a complex data structure and a new one is created, it might cause some components to re-render lower in the tree if their shouldComponentUpdate does reference equality.

Yes. This would also be true.

@TryingToImprove

This comment has been minimized.

TryingToImprove commented Jun 7, 2018

I don't see why it have to be treated as a partial update if prevState is returned. It would be possible to check the object reference instead of null? Otherwise a warning would be great.

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Jun 7, 2018

It would be possible to check the object reference instead of null?

Someone might modify the state parameter object. (This isn't the way you're supposed to use the parameter, but...)

@TryingToImprove

This comment has been minimized.

TryingToImprove commented Jun 8, 2018

I would suggest to add a warning about this. My first thought would be to return the prevState if there were no changes.

I tried myself, but could not find where to add an test.

*ReactPartialRenderer.js L:467
-----------------------------------------------------------------------------------------------
          if (partialState === inst.state) {
            const componentName = getComponentName(Component) || 'Unknown';
            warning(
              '%s.getDerivedStateFromProps(): Returned the previous state. ' +
                'If no changes return null, to prevent unnecessary performance bottlenecks.',
            );
          }

It would also be possible to use Object.freeze or a Proxy on "DEV", to prevent users from mutating the prevState.

@giorgi-m

This comment has been minimized.

giorgi-m commented Aug 17, 2018

@bvaughn I didn't follow new react features thoroughly yet, but a question. You say

Because the above methods might be called more than once, it's important that they do not contain side-effects. Ignoring this rule can lead to a variety of problems. For example, the code snippet you showed above might fetch external data unnecessarily.

AFAIK this was not the case with old react right? (that those life cycle methods could be called many times). Question is does what you stated above happen always in new react, or only in case I opt in to use async rendering?

Also can you expand a little bit on this:

For example, the code snippet you showed above might fetch external data unnecessarily.

OP had checks in order to compare userid from previous props to userid from new props, so why would data be fetched unnecessarily? Can you expand on this?

@j-f1

This comment has been minimized.

j-f1 commented Aug 17, 2018

Question is does what you stated above happen always in new react, or only in case I opt in to use async rendering?

I’m not on the React team, but it is likely that there won’t be an option, since async rendering is a performance improvement.

OP had checks in order to compare userid from previous props to userid from new props, so why would data be fetched unnecessarily? Can you expand on this?

What could happen with async rendering is that the app could start to re-render, then cancel the re-render and try it again later. This would cause the function to be called twice with the same previous and new props. I think.

@thejohnfreeman

This comment has been minimized.

thejohnfreeman commented Sep 2, 2018

Am I the only one who finds the new recommended pattern much less attractive than the OP?

  • Manually comparing userId for equality. OP handled this with distinctUntilChanged.
  • Manually cancelling requests. OP handled this with switchMap.
  • Two call sites for _loadUserData compared to one for fetchUserProfile.
  • Four special React methods (getDerivedStateFromProps, componentDidMount, componentDidUpdate, componentDidUnmount) compared to three.

I think much of the trouble with state-derived-from-props comes from the fact that React assigns the props after construction. I think I'd rather:

  • Have only the special methods ComponentDidMount (React seems to insist on this, though I'd rather drop it) and ComponentDidUnmount (effectively a destructor).
  • Initialize my state only in my constructor.
  • Have React reconstruct my component if my props need to change.

This has some big advantages:

  • The mental model of a component is much simpler with only two special methods (plus setState and render) and props that never change from under you.
  • shouldComponentUpdate gets a default implementation: if and only if the props changed.
  • I can assume the class invariant that my props never change. Then, for example, I can assume that my props at the time a response comes back are the same as they were when I sent the request.
  • I can use tools like MobX and RxJS to, in my constructor, define more derived/computed state without having to worry about how to reinitialize them in the other lifecycle methods.

At the very least, it might help to have a component adapter or base class (like PureComponent) that offers these guarantees.

@thejohnfreeman

This comment has been minimized.

thejohnfreeman commented Sep 3, 2018

My first shot excerpted below. Would love critique. Working implementation (and the rest of the details) at [CodeSandbox].

// The abstract base class for developers to extend.
// You get some observable props,
// and your responsibility is to present an observable element.
// You can observe `this.mounted` to react to `componentDidMount()`
// and `componentWillUnmount()`, if you please.
abstract class RxComponent<Props> {
  constructor(
    protected readonly props: Observable<Props>,
    protected readonly mounted: Observable<boolean>,
  ) {}
  element: Observable<JSX.Element>
}

// The adapter between the base class above and `React.Component`.
function rxComponent<Props> (
  Component: Constructor<RxComponent<Props>>
): Constructor<React.Component<Props>> {
  return class extends React.Component<Props> {
    private props$ = new BehaviorSubject<Props>(this.props)
    private mounted$ = new BehaviorSubject<boolean>(false)
    private component = new Component(this.props$, this.mounted$)
    public state = {element: null}
    private subscription = this.component.element.subscribe(
      element => this.setState({ element })
    )
    public componentDidMount() {
      this.mounted$.next(true)
    }
    public componentDidUpdate() {
      this.props$.next(this.props)
    }
    public componentWillUnmount() {
      this.subscription.unsubscribe()
      this.mounted$.next(false)
    }
    public render() {
      // We don't really care how many times React calls `render()`
      // because we don't spend any time in this function constructing a new element
      // and we return the exact same element since the last state change.
      return this.state.element
    }
  }
}

// OP's example, as an `RxComponent`.
@rxComponent
class RxExample extends RxComponent<{ match: Match<{ userId: number }> }> {
  public readonly element = this.props.pipe(
    map(props => props.match.params.userId),
    // Do not fetch unless the parameters have changed.
    distinctUntilChanged(),
    // Do not start fetching until after we've mounted.
    filterWhen(this.mounted),
    loadingSwitchMap(fetchUserProfile),
    map(({ value, pastDelay }) => {
      // Loaded.
      if (value) {
        return <p>user = {JSON.stringify(value)}</p>
      }
      // Taking a long time to load.
      if (pastDelay) {
        return <p>Loading...</p>
      }
      // Avoid flash of content.
      return null
    }),
  )
  // This doesn't build an element until the component is mounted.
  // If you care about what React shows between
  // constructing your component and mounting it,
  // then you can use `startWith`.
}
@felixfbecker

This comment has been minimized.

felixfbecker commented Sep 3, 2018

@thejohnfreeman

This comment has been minimized.

thejohnfreeman commented Sep 3, 2018

Very similar! I'll start a discussion in your project.

@receter

This comment has been minimized.

receter commented Sep 28, 2018

@bvaughn Maybe I miss something, but the suggested pattern might call _loadUserData multiple times if the component gets re-rendered while the AJAX request is waiting for a response.

I think of adding a comparison to prevState to prevent this behaviour.

  componentDidUpdate(prevProps, prevState) {
    if (prevState.profileOrError !== null && this.state.profileOrError === null) {
      // At this point, we're in the "commit" phase, so it's safe to load the new data.
      this._loadUserData();
    }
  }

What do you think?

@thejohnfreeman

This comment has been minimized.

thejohnfreeman commented Sep 28, 2018

@receter that is where RxComponent and switchMap come in very handy.

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Sep 28, 2018

@receter Seems reasonable.

@JustFly1984

This comment has been minimized.

JustFly1984 commented Nov 28, 2018

I have weird situation with gDSFP:

I have this code:

static getDerivedStateFromProps (props, state) {
    if (!state.loaded) {
      return new Promise((resolve, reject) => {
        const {
          googleMapsApiKey,
          language,
          region,
          version
        } = props

        injectScript({
          id: 'googlemaps',
          url: `https://maps.googleapis.com/maps/api/js?v=${version}&key=${googleMapsApiKey}&language=${language}&region=${region}`,
          onSuccess: () => {
            props.onLoad()

            resolve({ loaded: true })
          },
          onError: () => {
            resolve({ loaded: false })

            throw new Error(`
There has been an Error with loading Google Maps API script, please check that you provided all required props to <LoadScript />
  Props you have provided:

  googleMapsApiKey: ${props.googleMapsApiKey}
  language: ${props.language}
  region: ${props.region}
  version: ${props.version}

Otherwise it is a Network issues.
`
            )
          }
        })
      }).then(result => {
        console.log(result)

        return result
      })
    } else {
      this.props.onLoad()

      return {
        loaded: true
      }
    }
  }

It does return resolved value, but too late, and does not errors out. And state does not change.

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Nov 28, 2018

getDerivedStateFromProps does not support an async (Promise) return type. So this isn't expected to work.

@JustFly1984

This comment has been minimized.

JustFly1984 commented Nov 29, 2018

I got this, but there is no error, nor StrictMode notifies me about it.

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Nov 29, 2018

That's a good point. Maybe we can add a DEV warning for this case. I'll look into it.

Currently, what will happen is that React assumes you are returning an object with enumerable properties and it will mix that object into your previous state. Since the Promise you're returning has no enumerable properties, this will just do nothing– leaving your state unchanged (but without throwing).

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Nov 29, 2018

I mentioned this issue to the rest of the team this morning. We're going to keep our eye out to see if it trips up anyone else. If it looks to be common, we'll add a new DEV warning for it. 😄 Thanks for reaching out!

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