promise is undefined until first load/run

[Khartir]

In Version 8 the run method no longer returns a promise. Instead the promise prop should be used. However, when using useFetch, the promise property is undefined.
Here is a test that demonstrates this:

  test("defer=true allows using the promise", () => {
    let check = 0
    const component = (
      <Fetch input="/test" options={{ defer: true }}>
        {({ run, promise }) => (
          <button
            onClick={() => {
              run()
              promise
                .then(() => {
                  return (check = 1)
                })
                .catch(() => {})
            }}
          >
            run
          </button>
        )}
      </Fetch>
    )
    const { getByText } = render(component)
    fireEvent.click(getByText("run"))
    expect(check).toEqual(1)
  })

I'll also add a Pull-Request with these failing tests.

[ghengeveld]

Thanks! This must have been an oversight on my part. Please do open a PR ❤️

[Khartir]

Unfortunately I can only provide some tests that show the problem (see #93), I don't have a solution for it.

[ghengeveld]

Yes, thanks for that. I've reviewed your PR and given it some thought. I now think promise should not be undefined at the first render. Instead it should always be a Promise. We can do that by setting the initial value of promise to a Promise that never resolves or rejects. Then once run() is invoked we replace it with the actual underlying promise (which should eventually resolve or reject).

[ghengeveld]

@Khartir I've taken the liberty to rename this issue to focus on the value of promise to always be a Promise. I've already worked out a fix which confirms that useFetch itself is not the issue. Thanks a lot for reporting this. It's a bug also because it causes TypeScript to break. One more reason to write the entire thing in TypeScript. Expect a patch release soon.

[phryneas]

Wouldn't that be problematic as a promise that never resolves/reject would cause a memory leak if it is subscribed to?

On the other side of the same problem:
The current patchnotes recommend code like this:

const { run, promise } = useAsync(...)
promise.then(onData, onError)

Wouldn't that lead to the promise being subscribed to once for each render until the promise resolves?

I'm not really sure if it was a good idea to drop the promise-return-value from run :/ It looks a lot like a foot-gun with very problematic edge cases.

[ghengeveld]

Good point. I'm not sure how the garbage collector handles this situation. At least you'll get the same unresolved promise each time, but that's still not ideal.

That snippet is indeed not optimal. Subscribing to a promise should be done in useEffect, e.g.:

const { run, promise } = useAsync(...)
useEffect(() => {
  promise.then(onData, onError)
}, [promise])

I'll update the notes accordingly. However, it won't work with <Async>{({ promise }) => promise.then(onData, onError)}</Async>, so we should come up with a better solution.

[phryneas]

Hm. Maybe we could put more focus on onResolve/onReject/onCancel and leave the promise as an internal implementation detail?
(Assuming those always just hold one callback function, haven't checked the implementation yet, just brainstorming here.)

It would be a little more clumsy to use with formik (see my example in #75 ) - but without the promise returned by run I can't think of a really straightforward way to do that anyways.

[bogdansoare]

also having this issue, run should return a promise

[ghengeveld]

@bogdansoare Could you explain your use case? That will help me think of any alternatives that might also work for you.

[bogdansoare]

my use case is having to perform an action after the run completes, for a concrete example I have a form inside a modal dialog and I want to close the modal when the run completes

<Form
        onSubmit={async values => {
          try {
            await run({
              name: values.name,
            });

            onClose();
          } catch (error) {
            console.log(error);
          }
        }}

[ghengeveld]

For this you should just be able to use onResolve callback.

[phryneas]

I think the puzzle piece missing here is to be able to pass something to onResolve when calling run. At least, that would be great for my example in #75, where actions.setSubmitting is only available within the callback (that promise was very useful there)

function MyForm() {
  const { run } = useFetch(
    "https://example.com",
    { method: "POST" },
    { defer: true }
  );

  return (
    <div>
      <h1>My Form</h1>
      <Formik
        initialValues={{ name: "jared" }}
        onSubmit={(values, actions) => {
          run(
            { body: JSON.stringify(values) }, 
            { onResolve: () => actions.setSubmitting(false) }
          );
        }}
        render={props => (
          <form onSubmit={props.handleSubmit}>
            <Field type="text" name="name" placeholder="Name" />
            <button type="submit">Submit</button>
          </form>
        )}
      />
    </div>
  );
}

[bogdansoare]

@ghengeveld onResolve it's out of the context of the submit function, so what happens if I want to call multiple runs one after each other with some logic in between the calls

[ghengeveld]

@phryneas I'm open to changing the run API to accept config overrides (or composition?) like that. However it will be a breaking change because currently run takes any number of arguments which are passed to deferFn as an array. Of course we could be very dynamic and try to see if any of these contains an object with onResolve on it, but that's fragile and impractical/impossible in TypeScript.

[ghengeveld]

@bogdansoare I feel your pain. I'm actually inclined to restore the chainability of run by returning a Thenable (PromiseLike) from it. Because it's not an actual Promise it avoids the uncaught exception issue (although await will still throw on rejection because it treats the Thenable as a Promise). However I think we should be careful not to regress in previously resolved problems. It's clear that the promise prop is causing more harm than good.

[phryneas]

@phryneas I'm open to changing the run API to accept config overrides (or composition?) like that. However it will be a breaking change because currently run takes any number of arguments which are passed to deferFn as an array. Of course we could be very dynamic and try to see if any of these contains an object with onResolve on it, but that's fragile and impractical/impossible in TypeScript.

@ghengeveld yup, that's a little unfortunate - this could be mitigated though by placing the options argument to run at the front as an optional signature overload - something like this:

function run<T extends any[]>(optionOverride: Partial<Options>, ...args: T);
function run<T extends any[]>(...args: T);

[bogdansoare]

@ghengeveld that sounds great, would be very helpful

[Khartir]

I'm currently trying to understand what the desired behavior of run returning a PromiseLike is and why it doesn't just return a Promise. (Which as far as I understand #79 was the reason the return-type of run was changed to undefined)
I assume the goal is to prevent run from throwing an Error in case there is no catch attached.
@ghengeveld Is that correct?

Would the better solution not be to only throw if there is neither a catch nor an onReject?
I was thinking of the following behavior:
When the Promise of the deferFn resolves:

1. Call onResolved if defined.
2. Let the Promise returned by run resolve with the result of the Promise returned by the deferFn

When the Promise of the deferFn rejects:

1. If onReject is defined:
   1.a. Call onReject
   1.b. Let the Promise returned by run resolve with the result of onReject
2. If onReject is not defined:
   2.a. Let the Promise returned by run reject with the error of Promise thrown by the deferFn

In other words,

• if onReject is defined, run returns a PromiseLike and never rejects,
• if onReject is not defined run returns a Promise and can rejects.

I think it's acceptable to have run throw an unhandled error, if the user does not implement onReject or catch for it.

If this is the desired behavior, a case could be made that in the non-rejecting case, run should resolve with the result of onResolved, but that can be discussed later on.

[phryneas]

I think I already suggested this over in react-async/future, but it might fit here, too:

An alternative would be to resolve in all cases (thus not triggering an error) and resolving either with {state: "success", value: /*...*/ }, {state: "cancelled"} or {state: "error", error: /*...*/ } - while this, too, would be an API break, is would expose both cases and never trigger an uncaught error.

[ghengeveld]

I've decided to just fix the issue at hand (promise being undefined), and care about the repercussions later. I've simply added a warning to the readme that you have to specify a rejection handler when chaining on promise.

As for the ongoing discussion, let's take that into account while designing the future version of React Async / Async Library. One idea I have right now is to introduce a then prop which is synonymous to promise.then, but specifies a default value for onReject, so it will always be defined even if you forget to provide it yourself. Essentially this:

const noop = () => {}
const then = (onResolve, onReject = noop) => promise.then(onResolve).catch(onReject)