forge42dev / remix-hook-form

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

fix: getFormDataFromSearchParams behavior #28

Closed w00ing closed 10 months ago

w00ing commented 12 months ago

Description

When making a GET request using submitConfig: { method: 'get' } (while passing handleSubmit to onSubmit property of <Form/>), I noticed that search params were serialized in URL like this:

http://localhost:3003/main/building-products?formData=%7B%22page%22%3A1%2C%22pageSize%22%3A5%2C%22userOrBuildingType%22%3A%22USER_INFO%22%7D

Which is decoded as 'formData={"page":1,"pageSize":5,"userOrBuildingType":"USER_INFO"}', thus representing a formData 'object' that contains key-value pairs of searchParams.

Given this, getFormDataFromSearchParams in src/utilities/index.ts (which assumed, I think, that search params were serialized in a standard way e.g. ?a=1&b=2) could not parse the search params properly. Consequently, getValidatedFormData on the same GET request did not work either.

I think the solution needs to be either:

  1. search params in a GET request should be serialized in a standard way (e.g. ?page=1&pageSize=5&userOrBuildingType=USER_INFO in my case)
  2. respect the structure that the entire search params query is contained in formData 'object', and just fix internal parsing logic.

I went ahead with the latter approach. The related test are failing (which is expected), and I do not have a clear idea on how to fix these tests.

Would love to hear your opinion on this issue :)

Fixes # (issue)

If this is a new feature please add a description of what was added and why below:

Type of change

Please delete options that are not relevant.

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Checklist:

w00ing commented 12 months ago

Digging deeper into the source code, I found out that onSubmit function inside useRemixForm utilizes the createFormData method:

https://github.com/Code-Forge-Net/remix-hook-form/blob/e0f5b1e92e42480b0da5a5012013651bf8c24c93/src/hook/index.tsx#L55C5-L61

Which stores the data from form inputs into an 'object' with key = formData, regardless of the request method.

https://github.com/Code-Forge-Net/remix-hook-form/blob/e0f5b1e92e42480b0da5a5012013651bf8c24c93/src/utilities/index.ts#L127-L135

This behavior is fine for POST requests, but with GET requests, it results in the query string that looks like the one I mentioned in the PR:

?formData=%7B%22page%22%3A1%2C%22pageSize%22%3A5%2C%22userOrBuildingType%22%3A%22USER_INFO%22%7D

Whereas I expected it to look like: ?page=1&pageSize=5&userOrBuildingType=USER_INFO, which is more or less a standardized way of formatting search params.

The structure itself, I think, is a matter of choice. However, storing all the search params in a single formData key could lead to unexpected behaviors when child routes also attempt to fire GET requests with different search parameters, expecting that the parent search params remain as they are.

So I my opinion, we should either fix the parsing logic of getFormDataFromSearchParams, or the form-creating logic of createForm to properly handle GET requests.

What are your opinions?

AlemTuzlak commented 11 months ago

@w00ing I was super busy with remix-development-tools v3 release and had 0 time to investigate this, I will be looking at this in the upcoming days and probably be merging it, just need to make sure it's working as expected!

AlemTuzlak commented 10 months ago

@w00ing I've merged a pr that fixes this issue, try it on v3