Add submitOptions argument for useSubmit and useFetcher

[kentcdodds]

This would allow folks to change their code from something like this:

const fetcher = useFetcher();

// ...
<button
  onClick={() => {
    fetcher.submit({ hi: "bob" }, { method: "post", action: "/some-path" });
  }}
>
  Submit bob
</button>

To this:

const fetcher = useFetcher({
  method: "post",
  action: "/some-path",
});

// ...
<button
  onClick={() => {
    fetcher.submit({ hi: "bob" });
  }}
>
  Submit bob
</button>

(While still supporting the old API as well).

I've seen some folks suggest they would actually prefer this API and I think I agree with them. Also, it would side-step the problem of fetcher being referentially stable (#9739).

I've already implemented this in @remix-run/react here: remix-run/remix#4882

Until this is implemented, the only way to reliably use a fetcher.submit in a dep array is to do something like this:

const fetcher = useFetcher()
const submitRef = useRef(fetcher.submit)
useEffect(() => {
  submitRef.current = fetcher.submit
})
React.useEffect(() => {
  // can safely use submitRef without including in dep array
  submitRef({other, deps}, {action: '/some-path', method: 'post'})
}, [other, deps])

The drawback here is that if we actually do depend on the defaultAction then it's possible we'll get the wrong one when submit is called.

This PR fixes that by allowing users to specify an action (among other submit options):

const fetcher = useFetcher({action: '/some-path', method: 'post'})
React.useEffect(() => {
  // can safely include fetcher in dep array
  fetcher.submit({other, deps})
}, [other, deps, fetcher])

As instructed, I've opened this discussion here because this is where the final implementation of fetcher code will live going forward.

Let me know how I can help. Hopefully it's easy to implement, document, and test because I've already got that PR as a reference.

Thanks!

[brophdawg11]

As a note, this closes remix-run/remix#4872 in Remix

[kentcdodds]

Oh right, I copied a bunch for both the issue and PR and forgot to mention that 😅 thanks!

[ryanflorence]

This was actually my first design for useFetcher, but then fetcher.Form showed up.

Init vs. call site conflicts?

What are the semantics when the init and the call site are different?

function Thing() {
  let fetcher = useFetcher({
    method: 'post',
    action: '/some-path',
  })

  return (
    <fetcher.Form method="get" action="/other-path">
      <button name="hi" value="bob" />
    </fetcher.Form>
  )
}

Future API

I've also been planning to use the init for "events" but haven't had a chance to write out the proposal yet.

useFetcher({
  onStart() {},
  onError() {},
  onSuccess() {},
  onComplete() {},
});

But those might be better configured at the call site too: fetcher.submit(data, { onError }) and <fetcher.Form onError />

Please don't turn this thread into a comment about these events, just want to point out that there might be something else we want to use the fetcher init for.

Fetcher Render Stability?

I'm unclear on this part. fetcher is highly memoized and only changes when its state changes. You can safely put it in a deps array and it'll be stable between renders like I've done here:

https://stackblitz.com/edit/node-zryjnr?file=app/routes/index.tsx

import { useFetcher } from '@remix-run/react';
import { useEffect, useState } from 'react';

export default function Index() {
  let fetcher = useFetcher();
  let [effectCalls, setEffectCalls] = useState(0);
  let [updates, setUpdates] = useState(0);

  useEffect(() => {
    setEffectCalls(effectCalls + 1);
  }, [fetcher]);

  return (
    <div style={{ fontFamily: 'system-ui, sans-serif', lineHeight: '1.4' }}>
      <h1>Effect Calls: {effectCalls}</h1>
      <button onClick={() => setUpdates(updates + 1)}>
        Internal Updates: {updates}
      </button>
    </div>
  );
}

Clicking the button does not cause the effect to run. fetcher is already stable.

[kentcdodds]

I think it's fine if we want to handle the case where it remains stable even if the default action changes (thought it's possible that could lead to bugs). But the addition of the API is desirable for a lot of people as well (myself included).

[ryanflorence]

I was responding to:

Until this is implemented, the only way to reliably use a fetcher.submit in a dep array is to do something like this:

I think there are two proposals in one here:

• Make fetcher.submit/load/Form stable across location changes (which we should do)
• Changing the signature of useFetcher

[kentcdodds]

Good point. I can make a second proposal if you'd like so they can be tracked and discussed separately.

[ryanflorence]

That would be great. The "stable across location changes" should be an issue that we tackle, so you can just remove the bits about render stability in this proposal.

[kentcdodds]

Yep 👍 done: #9739

[ryanflorence]

Alright, I've thought about this for a bit and I'm not super interested in it.

Since we've established that stable "submit" across location changes is not related to the signature change, I'm not sure what the value prop is here other than a bit of API preference.

Technically you need call site options, but you don't need init options. So a signature change is a "Convenience API", which we tend to shy away from.

We've also talked about passing an app supplied id to fetchers so they're easier to find in useFetchers elsewhere (like useFetcher(id?)). So the decision here is not just "is this convient enough to add" but rather "is this useful enough that it's worth taking over the signature of useFetcher for future things that aren't possible today?".

In other words, is a convenience API worth preventing (or complicating) future use cases? I'd say no.

I'm pretty sure all you need is this if you want it:

function useFetcherWithDefaults(defaults) {
  let fetcher = useFetcher();
  return useMemo(() => {
    return {
      ...fetcher,
      submit: (formData, options) => {
        return fetcher.submit(formData, { ...defaults, ...options });
      },
    }
  }, [fetcher]);
}

[kentcdodds]

So a signature change is a "Convenience API", which we tend to shy away from.

That's a fair point.

Though unfortunately the implementation you've offered will have stale closure problems so you'll need to either require that people memoize the defaults argument they pass or you'll have to do the latest ref thing which is annoying and potentially prone to bugs 😬

[ryanflorence]

Yep, they are defaults, I'd intentionally only take the first ones ever passed in and ignore every thing after. I wouldn't consider that a bug. Same use case as <input defaultValue />. It gets captured once and ignored ever after.

[kentcdodds]

I can get on board with that 👍

[chaance]

So the decision here is not just "is this convient enough to add" but rather "is this useful enough that it's worth taking over the signature of useFetcher for future things that aren't possible today?".

This could be solved easily if the ID was a param in the options object, a la useFetcher({ id }). Still thinking about how I feel on the proposal overall, but named params are arguably more clear (and future-proof) anyway.

[MichaelDeBoey]

This should now be fixed by #10336

[brophdawg11]

Sort of ;). This Proposal got forked a bit in the discussion above, so #10336 only addresses only a portion of this Proposal which was extracted to it's own separate bug (#9739) - see the above comment for context.

#10336 addresses #9739 (fetcher.submit render stability) but does not address adding method/action support to the useFetcher() call-site.