molecule-dev/molecule

Await returned promises for functions passed to setState/extendState

Closed this issue · 3 comments

This works:

extendState(new Promise((resolve, reject) => {
  // ...
}));

This doesn't (type error):

extendState(async () => {
  // ...
});

I'm guessing you want to execute a function immediately if it is passed to extendState?

The extendState function expects an instance of a Promise, and async () => {} is a function which returns an instance of a Promise, so the argument passed in your examples are not quite the same thing.

I.e., async () => {} is essentially () => new Promise(), not new Promise().

So if you want to use async () => { ... }, you should define the function first and call it:

const asyncFunction = async (foo: string) => {
  return { foo }
}

extendState(asyncFunction(`bar`))

Or you can make it anonymously inline:

extendState((async (foo: string) => {
  return { foo }
})(`bar`))

Your inclination to pass async () => {} does seem pretty natural though, so I will consider making that an option. The only drawback I can think of at the moment is that the async function wouldn't be able to accept any arguments, but I suppose that could be up to the developer to manage them within the parent scope. The case where the extended state should be a function of the previous state (i.e., extendState(state => ({ foo: state.foo.toUpperCase() }))) should also be taken into account.

I'll look further into this when I get around to #1.

On further inspection, if we added this functionality, the async function would always receive the current state - i.e., extendState(async () => {}) would always ultimately be extendState(async (state) => {}). It would be up to the developer whether or not the state is used at the start of the async operation.

So if the real current state is needed with the actual update (maybe it was updated by something else during the async operation), we'd need to allow something like the following:

extendState(async (possiblyStaleState) => {
  const data = await fetchData()

  // At this point, `possiblyStaleState` might not be the current state, so...
  return (guaranteedCurrentState) => {
    return {
      foo: guaranteedCurrentState.foo ? data.bar : data.foo
    }
  }
})

I've decided against enabling this functionality for now, as it introduces more complexity than I would like.

If you really think this functionality should be included, I'm happy to reopen the issue and discuss it further.

In the mean time, I think the following usage is the way to go:

const foo = async () => {
  return { foo: `bar` }
}

extendState(foo())

I'll also add a note to the blog post about the hook(s) about this potential misconception.