forge42dev / remix-hook-form

Open source wrapper for react-hook-form aimed at Remix.run
MIT License
330 stars 27 forks source link

(bug): Remix hook form is not behaving as expected or documented #80

Closed AdiRishi closed 2 months ago

AdiRishi commented 6 months ago

I've encountered a problem with remix-hook-form where form submissions are not behaving as expected or as I believe is documented. My apologies in advance if this issue stems from a misunderstanding on my part. However, after several attempts and reviews of my code, I'm inclined to believe there might be an underlying issue with how remix-hook-form handles form submissions, specifically when trying to use the method="post" and action attributes without explicitly using a useFetcher.

Overview

Issue reproduction

I've made a repository that demonstrates the problem. You can find it here - https://stackblitz.com/edit/remix-run-remix-xr6vlj?file=app%2Froutes%2F_index.tsx

Key Points:

Conclusion

Again, I'm very sorry if I'm being dumb and missing something obvious, but I can't for the life of me figure out what is going wrong. Looking forward to finding out more 🙏

AlemTuzlak commented 6 months ago

@AdiRishi So the reason your form does not work as you'd expect it is because the handleSubmit that is passed to the Form component actually uses useSubmit under the hood and submits to the current location, the extra submitConfig is basically used to override this behavior for the useSubmit. The method and action should be there as well in case JS isn't loaded but otherwise it would work as expected if you add the config.

The isSubmitSuccessful mimics the behavior from react-hook-form where it is set as soon as the data is SENT to the server. isSubmitting is used to determine if the submission is still going on.

So these are not bugs per say, but could be potentially improved upon. It's very hard to determine on the hooks end if the submission was successful. What if you return 200 with errors, or redirect to somewhere. Although I think it would maybe be possible to access the form event and get the method and action instead of having to define it via config though.

AdiRishi commented 6 months ago

@AlemTuzlak thanks for looking into this. Do you think it's reasonable for the library to try and extract the method + action from the form directly? I'm fairly certain you would be able to reach it via event.target?

Just a little more clarification. As you said, when we add the submitConfig option to SubscribeFormThatDoesNotWork it does indeed post to the correct location. But it also does a page reload and moves to that action route. Is that expected default behavior? It seems like we need to specify a fetcher and use fetcher.Form in order to get it to submit the form via ajax requests

AdiRishi commented 6 months ago

Re your point on determining success vs failure. Yeah it makes sense, there are many scenarios that could mean success or failure. Generally speaking however I think it seems reasonable to assume a non-2xx response is a failure.

I can always get around this by try/catch on the action and make sure there is no uncaught error. What is the canonical way to return a "route failure" to the form with this library?

AlemTuzlak commented 6 months ago
  1. It's more than reasonable and I'd prefer that than the current API tbh. I'll look into it when I get time, if you're in a rush feel free to create a PR. (Action and method I mean)
  2. The usual way is to return the errors object or create it yourself and return it and the hook will pick it up and use it to show errors
AdiRishi commented 6 months ago

Thanks! I've learned a bit more since I made this issue, seems fetcher doesn't really return error states since it propagates that up to the error boundary. Maybe it makes sense for us to provide a helper error boundary that understands form errors or "top level" errors that can be thrown from the API.

Leaving the issue open since I plan to revisit this and maybe make a PR if I find a reasonable solution to these issues.

AlemTuzlak commented 2 months ago

@AdiRishi I implemented this behavior in v5.0.0