-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Allow react components' lifecycle methods to be async #1803
Comments
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 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');
})();
}
} |
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 |
cc @thejameskyle :> |
On the other hand, if React doesn't care about returned value shouldn't it be |
I think this could actually be misleading, like react is actually going to wait until promise is resolved |
I think this isn't actually a great idea, even without issue with Flow. We should unsubscribe / cancel these async operations in So maybe Flow shouldn't encourage such pattern. |
There's precedent in callback functions to annotate the callback as 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 So I think there's some justification for changing the return type from Another option is to change the return type to |
One problem with using
This could also be unsafe in context of lib defs. In future versions of a library returning something other than |
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. |
any workaround for this? |
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? |
Here is the workaround: class X extends React.Component<{}> {
componentWillMount() {
this._componentWillMount();
}
async _componentWillMount() {
}
} |
Are there any negative consequences to using suppressions like |
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: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?
The text was updated successfully, but these errors were encountered: