edmundhung / conform

A type-safe form validation library utilizing web fundamentals to progressively enhance HTML Forms with full support for server frameworks like Remix and Next.js.
https://conform.guide
MIT License
1.8k stars 101 forks source link

Replace `unknown` in submission payload #706

Closed timvandam closed 1 day ago

timvandam commented 2 months ago

Fix #628

This pull request replaces the unknown type in submission payloads. unknown is not compatible with Remix Single Fetch and new defineLoader or defineAction as it cannot be guaranteed that values of type unknown can be sent over the wire.

Removing unknown types addresses this issue. I've replaced it with what I think is correct but I don't know the ins and outs of this library so plz check.

Thanks!

changeset-bot[bot] commented 2 months ago

🦋 Changeset detected

Latest commit: e44c9e76db0d5b19211ab3f1ff53833ebd054a0f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages | Name | Type | | ----------------- | ----- | | @conform-to/dom | Patch | | @conform-to/react | Patch | | @conform-to/yup | Patch | | @conform-to/zod | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

timvandam commented 2 months ago

This does not seem to work for some playground files because File cannot be JSONified, breaking useActionData. While File is never sent from server to client, this is not reflected in the type of SubmissionResult.reply(). What do you think of adding a new reply option that narrows the type to exclude files in the payload type? I think this would make types work with Remix Single Fetch/defineLoader/defineAction using syntax like result.reply({ excludeFiles: true })

Let me know if that works and I will change this PR @edmundhung

edmundhung commented 1 month ago

Thanks for the PR @timvandam. I was not aware that I can represent the submission payload this way, but now I see you did that it make total sense :+1:

What do you think of adding a new reply option that narrows the type to exclude files in the payload type? I think this would make types work with Remix Single Fetch/defineLoader/defineAction using syntax like result.reply({ excludeFiles: true })

We are stripping all files already if you are running on the server. So to make it non-breaking, we will need to default the new option to true. But I will just exclude File type on the SubmissionResult for now since only Conform's internal care about the File and it is something I plan to change soon.

I can't push any changes to your PR likely because your PR was based on your main branch. This is what I am thinking:

export type SubmissionPayload<Entry extends FormDataEntryValue> =
    | Entry
    | SubmissionPayload<Entry>[]
    | { [key: string]: SubmissionPayload<Entry> };

export type Submission<Schema, FormError = string[], FormValue = Schema> =
    | {
            status: 'success';
            payload: Record<string, SubmissionPayload<FormDataEntryValue>>;
            value: FormValue;
            reply(options?: ReplyOptions<FormError>): SubmissionResult<FormError>;
      }
    | {
            status: 'error' | undefined;
            payload: Record<string, SubmissionPayload<FormDataEntryValue>>;
            error: Record<string, FormError | null> | null;
            reply(options?: ReplyOptions<FormError>): SubmissionResult<FormError>;
      };

export type SubmissionResult<FormError = string[]> = {
    status?: 'error' | 'success';
    intent?: Intent;
    initialValue?: Record<string, SubmissionPayload<string>> | null;
    fields?: string[];
    error?: Record<string, FormError | null>;
    state?: SubmissionState;
};

Then, on replySubmission, we will do this:

const initialValue = normalize(
    context.payload,
    // We can't serialize the file and send it back from the server, but we can preserve it in the client
    typeof document !== 'undefined',
    // We need the file on the client because it's treated as the form value
    // But we will exclude the File type for now as it's only used by the internal
    // form state and we will remove the need to preserve the file on the client soon
) as Record<string, SubmissionPayload<string>> ?? {}

return {
    initialValue,
    // ...
};
timvandam commented 1 month ago

Awesome, not sure why you cannot push to my branch, but I've added what you've commented. Very nice solution with the generic, I was thinking of much more complex ways of going about it

pkg-pr-new[bot] commented 3 weeks ago

Open in Stackblitz

More templates

- [@conform-example/chakra-ui](https://pkg.pr.new/template/7fc0a602-23a6-48b0-a4c8-c1545c1001ce) - [@conform-example/material-ui](https://pkg.pr.new/template/8c815c8a-a9c7-48a6-b19d-964a28a0e25c) - [@conform-example/nextjs](https://pkg.pr.new/template/342f2b81-9ee6-43b6-99c6-55f1aebd7913) - [@conform-example/headless-ui](https://pkg.pr.new/template/05704c07-a2af-4066-914f-6d675bbfe573) - [@conform-example/react-router](https://pkg.pr.new/template/5300fb5a-9437-4443-9926-82a6cbb78cdc) - [@conform-example/radix-ui](https://pkg.pr.new/template/ef7a4adb-2453-4adf-89c9-71f1e1914eca) - [@conform-example/remix](https://pkg.pr.new/template/9a4a3959-cee4-4a3d-aa08-166ff6c9bd5d) - [@conform-example/shadcn-ui](https://pkg.pr.new/template/b41e9139-3318-4d4e-aff9-424c7cb382bf)

@conform-to/dom

``` pnpm add https://pkg.pr.new/@conform-to/dom@706 ```

@conform-to/react

``` pnpm add https://pkg.pr.new/@conform-to/react@706 ```

@conform-to/validitystate

``` pnpm add https://pkg.pr.new/@conform-to/validitystate@706 ```

@conform-to/zod

``` pnpm add https://pkg.pr.new/@conform-to/zod@706 ```

@conform-to/yup

``` pnpm add https://pkg.pr.new/@conform-to/yup@706 ```

commit: e44c9e7

JasonColeyNZ commented 6 days ago

Hi guys, when will this PR be released, this is very much needed right now for Remix and Single Fetch?

edmundhung commented 3 days ago

I am hoping to cut a release sometimes this month. If you can give the pre-release version a try and let me know whether you run into any issues, it would help me releasing this change sooner. Thanks!

JasonColeyNZ commented 3 days ago

I am hoping to cut a release sometimes this month. If you can give the pre-release version a try and let me know whether you run into any issues, it would help me releasing this change sooner. Thanks!

Sorry to sound dim, how do I do this, do I just change my package .json to ...

    "@conform-to/react": "pre-release",
    "@conform-to/zod": "pre-release",
edmundhung commented 2 days ago

There is a comment above from pkg.pr.new with each package name listed. You will find a command to install the pre-release version of that package if you expand the item.

If you are not using pnpm, you can swap it with npm install, or just copy the package URL and replace the version in the package.json with it.

Really appreciate for giving this a try! 👍