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

feat: allow actions to have zero or multiple inputs #48

Closed
wants to merge 1 commit into from

Conversation

bram209
Copy link

@bram209 bram209 commented Jan 4, 2024

not sure if this is the best approach DX wise, but having the ability to pass multiple arguments to actions let's you:

Pass additional arguments:

Use the useFormState hook:

allows the server’s response from submitting the form to be shown even before hydration has completed.

To demonstrate this, I added two examples (see form-with-bind and form-with-state)

Copy link

vercel bot commented Jan 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
next-safe-action-example-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 4, 2024 9:26am
next-safe-action-website ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 4, 2024 9:26am

@bram209
Copy link
Author

bram209 commented Jan 4, 2024

resolves #29

@@ -118,7 +118,7 @@ const useActionCallbacks = <const S extends Schema, const Data>(
* {@link https://next-safe-action.dev/docs/usage-from-client/hooks/useaction See an example}
*/
export const useAction = <const S extends Schema, const Data>(
safeAction: SafeAction<S, Data>,
safeAction: SafeAction<[S], Data>,
Copy link
Author

Choose a reason for hiding this comment

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

For now, I kept hook functionality as is, meaning that you can only use the hooks with actions that have exactly one input.

? { ...prev, ...buildValidationErrors(parsedInput.issues) }
: prev;
},
{} as Partial<Record<keyof InferArray<S>[number] | "_root", string[]>>
Copy link
Author

@bram209 bram209 Jan 4, 2024

Choose a reason for hiding this comment

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

I simply merge all validation errors together, but this is a naive approach. Different arguments could have fields that overlap and it is hard to see from which argument a validation error came from.

Option 1
We could simply only return the validation errors of the first input that failed and avoid breaking changes.

type SafeActionResult<S extends Schema[], Data> = {
	data?: Data;
	serverError?: string;
	validationErrors?: Partial<Record<keyof InferArray<S>[number] | "_root", string[]>>;
        validationErrorsArgIndex?: number; // ADDED - needs a better name, but you yet the idea
}

Option 2
Other (more type safe, but breaking) option would be to do something like:

type ValidationErrors<S extends Schema> =  Partial<Record<keyof Infer<S> | "_root", string[]>>
type SafeActionResult<S extends Schema[], Data> = {
	data?: Data;
	serverError?: string;
	validationErrors: { [K in keyof S]: ValidationErrors<S[K]> };
}

So that the user can access validation errors like:

const input1Errors = result.validationErrors[0] // full type safety here
const input2Errors = result.validationErrors[1]

Copy link
Author

@bram209 bram209 Jan 4, 2024

Choose a reason for hiding this comment

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

Personally, I would go for option 1 initially, considering this PR should not introduce any breaking change this way.

Option 2 would require a major version bump and users of the library would have to update their validation error handling logic through their codebase (however, this would be mostly done by a simple search & replace: result.validationErrors > result.validationErrors[0])

what are your thoughts on this?

@TheEdoRan
Copy link
Owner

First of all, sorry for the huge delay in my response. I've been very busy this past month, and this, as you probably already know, is a pretty difficult feature to implement. I also wanted to lay the foundation to support this feature before giving you feedback, and I think the updated validationErrors structure implemented in next branch is a right move.

I'm still trying to find the best way to support Form Actions in the library, along with standard Server Actions, which are still very important (I'm using next-safe-action + React Hook Form combo in all of my apps).

That said, since next-safe-action, in one way or another, will support them in v7, and since the major version bump comes from the validationErrors structure breaking change, we're not strictly required to follow the current API to avoid breaking other things, as the main focus of this library is to provide the best type safe way to deal with Server Actions to the user, and the Server Actions API itself is still pretty new.

I've read through your code, and even though everything works, I'm still considering whether this is the best way to implement this feature (not saying it isn't either). A different approach would be to provide an utility action wrapper function just for Form Actions, or maybe two, one for Server Form Actions and another one for Client Form Actions, along with a type safe wrapper for the useFormState hook for Client Form Actions.

If you still want to help me find the right way to support this functionality in the library, I'll gladly accept your contribution. Please let me know what you think about my thoughts and the different implementation options, and sorry again for the delay!

@TheEdoRan TheEdoRan changed the base branch from main to next February 7, 2024 23:21
@bram209
Copy link
Author

bram209 commented Mar 3, 2024

Don't worry about the delay, I understand that life & work comes first : )

A different approach would be to provide an utility action wrapper function just for Form Actions

I didn't check the next branch yet, but my first thought would be that you would run into the same requirement: passing zero, one or multiple additional argument(s) that are not part of your form.

Let's say you have a nested resource: /courses/1/questions/3, when your form wants to update the answer of question 3 from course 1, you would want to bind questionId and courseId.

You would still need something like an variadic function (approach in this PR), unless you do something like:

export const updateAnswer = formAction(
  additionalArgsSchema,
  formSchema,
  async (additionalArgs, { newAnswer }) => {
    console.log(
      `Updating answer for question ${additionalArgs.questionId} of course ${additionalArgs.courseId} with: '${newAnswer}'`
    );
    return {
      success: true,
    };
  }
);

where the user has to bind the additional args:

const action = updateAnswer.bind(null, { courseId: 1, questionId: 3 });

// pass action to form

But not sure if it is better?

@TheEdoRan
Copy link
Owner

Hey @bram209, first of all:

Don't worry about the delay, I understand that life & work comes first : )

Thank you.

So, I've thought a lot about the best way to implement Form Actions in next-safe-action. I've come to the conclusion that, in my opinion, the best approach is to have both variadic functions to support argument binding and a wrapper function for Form Actions used in Client Components, which require initial state as the first argument of the function.

The wrapper (let's call it formAction as you did) is necessary to:

  1. enforce the return type of the action when it's declared;
  2. exclude prevState from the returned validationErrors array.

What I don't really like is returning an array of validationErrors, I much prefer the current single object, but I guess this is the only proper way to handle them, if we need to support binding arguments, which we do.

Please let me know what you think about all of this, and since this will be a core feature of next-safe-action, the valuable thoughts of @varna, @benjick, @rwieruch and @theboxer would be greatly appreciated as well. Thank you!

@TheEdoRan
Copy link
Owner

TheEdoRan commented Mar 7, 2024

This PR is about supporting a variable number of arguments in Server Actions, but since that's strictly related to Form Actions, and we've already talked about it here in the comments above, here's an update on the Form Actions implementation:

the React team "intends to fix some of the confusion and limitations of the useFormState hook", by introducing a new hook called useActionState, that replaces the previous useFormState. Here are my thoughts.

@theboxer
Copy link
Contributor

@TheEdoRan sorry for a late response, I was going through it, stashed it and forgot to get back to it....

I don't really use React's form hooks or had a need to bind values to the server actions. I pretty much always go with react-hook-form and the current implementation of next-safe-action covers all my needs there.

I'd agree with your conclusion. Not 100% sure how the usage of the safe action will have to change for folks like me, who won't need to bind an extra arguments or use the form hooks. I'd hope it wouldn't change (much) :)

@TheEdoRan
Copy link
Owner

Closed in favor of #97

@TheEdoRan TheEdoRan closed this Apr 8, 2024
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.

3 participants