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

fix: handle framework errors properly in middlewares. #320

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dddddl12
Copy link

@dddddl12 dddddl12 commented Feb 5, 2025

Although #303 has been resolved, I had also been investigating the same issue. During my analysis, I noticed that framework errors were not being caught in the most appropriate location, resulting in failure to retrieve all expected values from next() when success=true.

To illustrate the problem clearly, I added a related test in commit efe6a96. This test demonstrates that the existing code does not pass. I then applied a fix in commit ec108ae, which resolves the issue.

Copy link

vercel bot commented Feb 5, 2025

@dddddl12 is attempting to deploy a commit to the Edoardo Ranghieri's projects Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

vercel bot commented Feb 5, 2025

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

Name Status Preview Comments Updated (UTC)
next-safe-action-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 10, 2025 4:32am

@TheEdoRan
Copy link
Owner

Hi and thanks for your contribution. I tested your code and, while using redirect works fine in the action method, the problem is that it doesn't work at all when called in the use method. If you try to use redirect inside a middleware function, the Next.js internal error doesn't get re-thrown and instead a serverError gets returned.

Being able to redirect inside a middleware function is a pretty important feature, since it allows to skip code execution if, for example, a user is not logged in. How would you solve this? I'll leave an example below.

Test action

const testAction = ac
  .use(async ({ next }) => {
    redirect("/");
    return next();
  })
  .action(async () => {
    return null;
  });

Output

image

@dddddl12
Copy link
Author

dddddl12 commented Feb 10, 2025

@TheEdoRan Thank you for your feedback! I've pushed a new commit to resolve the issue along with an additional test for it. To reduce code duplication, I created a separate class to handle framework errors. If you have any concerns or would prefer a different approach, please let me know.

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.

2 participants