Flow: Allow react components' lifecycle methods to be async

[jakpaw]

I tried to declare componentWillMount in a component as async but got a flow error: Promise This type is incompatible with undefined. I found out that it's caused by this definition:

componentWillMount(): void;

in https://github.com/facebook/flow/blob/master/lib/react.js

Shouldn't it be possible for components' lifecycle methods which return void to return a Promise instead?

[Macil]

I find the strictness about void functions useful as it's caught lots of cases where I've misunderstood a method or callback and tried returning what I thought was an important value.

Or if the React definitions were changed to have the return type of void|Promise, I think that would imply that React handled promises specially. I just had to double-check that wasn't the case.

You could still do async stuff inside a void returning function by using an immediately-invoked async function:

class X extends React.Component {
  componentWillMount() {
    (async ()=>{
      console.log('foo');
      await delay(123);
      console.log('bar');
    })();
  }
}

[brentvatne]

I agree with the original post, I think flow should let us do async lifecycle functions in React. See https://twitter.com/Vjeux/status/772202716479139840

[brentvatne]

cc @thejameskyle :>

[andreypopp]

Or if the React definitions were changed to have the return type of void|Promise, I think that would imply that React handled promises specially.

On the other hand, if React doesn't care about returned value shouldn't it be *?

[vkurchatkin]

I think this could actually be misleading, like react is actually going to wait until promise is resolved

[rpominov]

https://twitter.com/Vjeux/status/772202716479139840

I think this isn't actually a great idea, even without issue with Flow. We should unsubscribe / cancel these async operations in componentWillUnmount. Also person who reads async componentDidMount may assume that React does something with returned promise.

So maybe Flow shouldn't encourage such pattern.

[samwgoldman]

There's precedent in callback functions to annotate the callback as mixed when the caller doesn't care. The justification there is that () => e may be used solely for the side-effects of e. If e has a return value, it doesn't hurt anything to return it. This lifecycle method is a callback in spirit, as far as I can tell, so any reasoning that applies to callbacks _should_ apply here.

I'm of two minds here, really. On one hand, I think Flow should concern itself with type _errors_ primarily and avoid too much hand-holding. If something works at runtime, we should have a good reason to make it a type error.

On the other hand, I _personally_ believe that () => void is an useful signifier of "side effects are happening here" and changing that to () => mixed communicates poorly by not guiding developers away from useless code like [1, 2, 3].forEach(x => x + 1). For my own code, I would use () => void, but I understand why the community decided to go the other way.

So I think there's some justification for changing the return type from void to mixed.

Another option is to change the return type to void | Promise<void>, but I don't like that idea. That type signature gives the impression that React will work to make async side effects safe, but it won't. I think it's misleading / doesn't represent the "actual" types.

[vkurchatkin]

One problem with using async function as callback, is that it's easy to forget, that rejections will be ignored. If callback is () => void, you are forced to think about it.

There's precedent in callback functions to annotate the callback as mixed when the caller doesn't care

This could also be unsafe in context of lib defs. In future versions of a library returning something other than undefined could have a special meaning which could lead to unexpected behaviour.

[echenley]

Probably worth mentioning that this is slightly annoying for tests. If you want to do something like:

componentDidMount() {
  return doSomething().then(() => {
    this.setState({ some: 'state' })
  })
}

An ideal enzyme test would be something like this:

it('fetches something on componentDidMount then sets state', async () => {
  const component = shallowRenderComponent()
  await component.instance().componentDidMount()
  expect(component.state()).toBe({ some: 'state' })
})

But because of this flow restriction, you instead have to split out the async logic into another testable method:

componentDidMount() {
  this.doSomething()
}

doSomething() {
  return doSomething().then(() => {
    this.setState({ some: 'state' })
  })
}

And the test becomes:

it('calls this.doSomething on mount', () => {
  const component = shallowRenderComponent()
  const doSomething = jest.fn()
  component.instance().doSomething = doSomething
  component.instance().componentDidMount()
  expect(doSomething).toHaveBeenCalled()
})

it('doSomething fetches something then sets state', async () => {
  const component = shallowRenderComponent()
  await component.instance().doSomething()
  expect(component.state()).toBe({ some: 'state' })
})

Not a deal breaker, but kind of a burden for seemingly not much gain.

[sibelius]

any workaround for this?

[akoufa]

Hello and thank you for the flow type checker. I would like to know if there is a solution for this issue or a workaround?

[vkurchatkin]

Here is the workaround:

class X extends React.Component<{}> {
  componentWillMount() {
    this._componentWillMount();
  }

  async _componentWillMount() {

  }
}

[Palisand]

Are there any negative consequences to using suppressions like $FlowFixMe instead of an IIFE (@AgentME) or a sub-function (@vkurchatkin)? Wouldn't this be the easiest and fastest workaround provided you have already setup your _.flowconfig_ to support it?