Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace unknown in submission payload #706

Merged
merged 1 commit into from
Sep 12, 2024
Merged

Conversation

timvandam
Copy link
Contributor

@timvandam timvandam commented Jul 14, 2024

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!

Copy link

changeset-bot bot commented Jul 14, 2024

🦋 Changeset detected

Latest commit: e44c9e7

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 timvandam force-pushed the main branch 2 times, most recently from 7b3b246 to 1c82e88 Compare July 14, 2024 08:57
@timvandam
Copy link
Contributor Author

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
Copy link
Owner

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 👍

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 timvandam force-pushed the main branch 3 times, most recently from 615031b to 33d1223 Compare August 9, 2024 18:30
@timvandam
Copy link
Contributor Author

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

Copy link
Owner

@edmundhung edmundhung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love this change. Thanks!

Copy link

pkg-pr-new bot commented Aug 17, 2024

Open in Stackblitz

More templates

@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
Copy link

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

@edmundhung
Copy link
Owner

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
Copy link

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
Copy link
Owner

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! 👍

@edmundhung edmundhung merged commit 3358b54 into edmundhung:main Sep 12, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remix single fetch: returning submission.reply() with action results in type never.
3 participants