If a function wrapped in action throws some error it should be possible to catch it. about solid-router HOT 20 CLOSED

Yeah I just went with my original proposal. .error field and throw when not form. Tell me if this works for you well. Released in 0.13.0

from solid-router.

Yeah I've been staring at this issue the past couple days and having some difficulties coming up with a solution I like so I want open up the discussion here on what I'm thinking. This starts with a few assumptions that I want to make about the solution.

1. If we provide a declarative solution for handling errors it should always work regardless of the method in which the action is invoked. Ie. if there is a .error field in the submission it should be able to work regardless.

2. fetch doesn't error unless the request fails. Status codes even 500s don't throw. Which means when considering network traffic especially with server functions there is no real difference between us throwing something or returning it unless the decision is to not respond on the server when a irreconcilable error is thrown.

3. useAction gives us insight into where an action is used but it grabs the context of where that "hook" is and not where it is ultimately called. Forms have no such hook in but inject the router via events it sets up. In so error boundaries aren't really a great solution. There is no "read" for actions. Yes you can read the returned results, but that is optional.

Starting with these assumptions it is difficult to have a single behavior that satisfies everyone. For instance throwing an error on the submit function means that one can't rely on a .error field and will have unhandled errors if they don't handle it. We could say that this only applies to useAction invocations and not forms, but if you handle the error it doesn't change the fact the submission has error'd. Like it wouldn't clear .error field (or prevent it from being set). That could only be done from the inside.

Having a .error field at all is interesting given the way fetch works when considering server functions. Would we throw on status codes? Like if response.ok is not set. My preference would be to avoid getting to that level. But then maybe a thrown error on the server should cause fetch to fail. That being said that might be very helpful to the client. Whereas we can return a more detailed response. That being said if you wouldn't expect to handle the error we probably should address this earlier in the chain. But then the client isn't getting a helpful error and the .error field seems wasted.

The one commonality I see in all this is we almost never want to actually throw known errors from user code. Because either it won't propagate and we won't get valuable information.. say like if it is a validation issue you might want to communicate that, or if we do communicate the error over HTTP then throwing afterwards is arbitrary and so baking that in doesn't seem to make a ton of sense. Throw is actually awkward in the latter because it removes our knowledge of what are legitimate responses we are dealing with (from TS or otherwise).

I'd love to have more discussion on this topic and what people would want to see.

from solid-router.

I have some thoughts about the points you've mentioned.

1. Why it should work if an error occures?

There're 2 options to call actions:

with useAction
Here we can intercept an error with catch, so we have full control on it. I'm curious about what data should be passed to useSubmission. I guess, submission should have status field instead of pending, because it might have an error:

// smth like this
type Submission = {
   status: "pending";
} | {
   status: "error";
   error: unknown;
}

pass action directly to the form
Here's interesting case. we can't provide catch handler here, but you've found a solution to bind an action to some arguments with action.with(...). Maybe it's possible to do smth similar to pass an error handler: action.with(...).error(() => ...)

TBH I don't like this approach, because it separates the same action (error handling) with different syntax. So we need a unified mechanism which can be applied in any scenario. What we need to do? Handle errors for actions, so it means, this should be applied to the action itself (and not to useAction thing).

Maybe we can go with the following:

// make an action outside of a component

const myAction = action(() => {...});

// in a component:
// here it's possible to add several event listeners, because we can use the same action in different places and handle errors differently.
// in this case we also need to support `off` method.
myAction.on('error', (error: unknown) => {...});
// and only then you can use it with `useAction`

With this approach we don't need to think about 3 case

from solid-router.

About fetch thing.

Yes, fetch doesn't error, but many libraries developers use do (like axios, redaxios). Personally, I use my own solution, and even there I added throwing error on some http codes, because it's quite convenient. You can get the result of the request from then and otherwise handle error in catch. I think, Developers will still prefer this behavior (I'm not even sure, that many developers know this fact about fetch 😆)

from solid-router.

Interesting, if a server action fails (for instance, there was an error when trying to write to a DB), why shouldn't it error?

from solid-router.

Anyways, it's not a simple question though. If an action never fails, eventually we need some way to handle errors somehow. Another take here: the complexity partially comes from 2 different ways of calling actions: one is passing an action directly to the form and another one is useAction for manual calls. What if we have just one way of doing them?

export function useAction<T extends Array<any>, U>(action: Action<T, U>) {
  const router = useRouter();
  return (...args: Parameters<Action<T, U>>) => action.apply(router, args);
}

useAction just passes a router to an action, so we need to initialize action with router context before calling it.

what if instead of useAction we could have: myAction.bindToRouter() inside a component, and then we can call it somewhere: myAction().then(...)?

from solid-router.

About fetch thing.

Yes, fetch doesn't error, but many libraries developers use do (like axios, redaxios). Personally, I use my own solution, and even there I added throwing error on some http codes, because it's quite convenient. You can get the result of the request from then and otherwise handle error in catch. I think, Developers will still prefer this behavior (I'm not even sure, that many developers know this fact about fetch 😆)

I probably have some bias here. I started in a system that didn't throw on status, then changed to it, and then changed back (superagent). Honestly I am not a fan at all on throwing on status and agree with the fetch spec here pretty wholeheartedly.

Assuming we have the option to throw from a reasonable context.. we still have a problem if you want to .then it. Because the context will still get the error before you. Like myAction.then() isn't what the router gets it is just myAction. So even if you tie it up neatly that isn't going to prevent it from throwing to an error boundary.

This is what makes me think throwing to ErrorBoundary in general isn't really a go to here. Either you need a declarative API like .error which makes basically no sense over the wire or we continue to use result. Which means that throwing errors in general shouldn't be the mechanism. It should be the exception. If you want to respond.. don't throw.

So perhaps we should just not handle thrown errors at all. And server functions should fail on them as well. API stays the same we just throw when there is an error that isn't a response and that's on you. I guess that leaves forms in the dust, but not sure if there is much more to do for them. We could throw them from the router .. I guess.. but even then it isn't really great. What you'd want is to pass them back in through the declarative result. And now I'm back at the beginning again.

from solid-router.

I looked at other solutions. Remix throws from the route section, but their actions are tied to the route section so that works. They don't throw the whole page. Next uses useFormStatus to declaratively put the errors back in.

I think what we need to do here is define something to represent a throw. So that the client can throw again. However I don't think in either case .catch the action is probably going to be the way to handle this. I'm leaning more towards the Next approach because we don't tie things to the route. In which case we would have .error but for TS reasons I wouldn't recommend throwing errors intentionally.

EDIT: I was wrong about next. I don't think they declaratively put errors back in. They expect you to use to return Errors just like Remix and Solid. And they do throw from the boundary if it isn't handled.

from solid-router.

Interesting.

I don't think in either case .catch the action is probably going to be the way to handle this

Why is that?
If an error occures of the Promise occures, everybody knows that it will appear in catch clause. So, you don't need to learn new ways. I think it's a big plus.

from solid-router.

Why is that? If an error occurs of the Promise occures, everybody knows that it will appear in catch clause. So, you don't need to learn new ways. I think it's a big plus.

Well we could throw but the recommended way of handling wouldn't be the catch. I suppose throwing isn't the recommended way at all so maybe it doesn't matter. I'm just pointing out that in a lot of cases you'd end up with an uncaught exception error in the browser even if everything was handled properly (ie using .error field) if we throw.

from solid-router.

Maybe, but we're talking about the actions, right?
Actions are promises in this case, so we can expect a result from then clause and error from catch clause. You mentioned that there's some kind of serialization error: the error needs to be serialized in case of server error and thrown on the client side again to make it work (roughly).

In my opinion action - is a network thing, so it happends on both sides (either it's client action or server action). For both cases we're making a request (for server actions it's RPC), and if an error occures on one side (on the server) it should also fail on the client, because this client-server interaction is just one semantic thing - action. That is it might be either succesfull or not. For me it's weird that on the server side it fails, but it doesn't fail on the client side.

Maybe I'm missing some concepts about your definition of actions.

from solid-router.

If it is a form you need to be able to handle it so there is no place to have a .catch all we can do is feed it back either through an error boundary or through declarative UI. I was thinking declarative was right but there is one other option.

We could force useAction for all actions. Right now it is only for ones that we use manually. But if we did it for forms too they would be able to throw up to their nearest ErrorBoundary. But then you catching the error would do very little because the whole app would have to handle the Error.

I just don't see how we could rely on the catch when you consider forms. Either we will be throwing up to the ErrorBoundary or we will be pushing it back declaratively. That being said every manual action looks like doSomething().then... the problem is it is detached from the tree.. so we need to forceably push it back in either case.

Which brings us back to whether these should be handled differently. Assuming we want manual actions to throw. I'm having a harder time deciding what is appropriate for forms. Especially if their behavior is to be so different.

EDIT: This is one option.

We always set it declaratively so you can read it off the submission. However, if it is a form we don't throw. I can tell when it is a form submission so we can just avoid throwing in that case. If you do the submission manually well it will throw. You can ignore it but that's on you, it will still be available through the submission as well.

The biggest downside of this is people who don't look for the error with forms will never know. Still throwing to the ErrorBoundary is almost what I'd never want. Also developers would need to check both fields, because it is always better to return errors than throw them. Returned errors can be typed. I almost feel like Remix and Next throw errors to the boundary just because it is a pain enough that most people will return them instead.

Yeah this still doesn't feel quite right.

from solid-router.

If it is a form you need to be able to handle it so there is no place to have a .catch

But it's not true...
You did find a way to pass parameters to declarative form, right?:

<form action={myAction.with(someParam)}>...</form>

It possible to have the same thing for errors:

<form action={myAction.with(...).onError(...)}>...</form>

The API of course can be different, but you got the point.

EDIT:
Yes, it won't be a catch in this case, you're right. But it will be kind of catch 😆

from solid-router.

Another a little bit confusing thing (for me of course) is that we have two actions: manual and declarative.
So, if I need to call an option manually, I need to make a new function (why?) which comes from useAction (which actually only binds a context).
Is it possible to have only one form of action? Declarative, manual - don't care, because it's just one thing. For me it's a bit weird to have different functions just because of the way devs write code.

I mean, maybe, it could be done like:

const myAction = action(() => {...});

<form action={myAction}>...</form> // declarative style
<form action={myAction.with({
   args: [...someArguments],
   catch: (error) => {...},
   // extra options which can come in the future
})}>...</form>

myAction.bindToRouter().with({...}) // in a component before calling it

myAction() // manual call

from solid-router.

this with thing is actually a bind call, and useAction is the same bind under the hood, so maybe it's possible combine these things?

from solid-router.

Hm, another idea came to my mind. If an action won't throw an error, maybe not make it async? It will receive an object with callbacks (as XMLHTTPRequest for instance). Then, for people it will be clear where to get the result and where to handle errors (and any extra stuff).

EDIT: And for convenience reason it could have toPromise() which will return promise (if a user wants to manually call the action) with then and catch handlers defined in with

from solid-router.

I should have said form is also meant to work with no js present, so like with works because it updates the url. Any sort of catch/onError doesn't make as much sense. It needs to feed back in declaratively or represent via ErrorBoundary.

from solid-router.

The thing is if I don't like ErrorBoundaries then the only option is a split between declarative and throwing. ErrorBoundaries are the only solution that could cover both but they are my least favorite option.

Thinking about what I proposed before the only thing I didn't like was having a separate error field. It feels odd. I wonder if we could get away with simply an error boolean. Then result just stays the same. Awkward for TS I suppose but it would still work the way it works now for forms except you'd know when it threw to get there and can identify the difference between known and unknown errors.

I think TS will be a blocker though. I just hate the experience of having to always check for .error just in case. It is really nice how Remix and Next basically through API's discourage throwing unless it is an unredeemable error.

from solid-router.

Seems quite reasonable, I need to play with it, but I think it's ok solution right now. It's better to have a way to handle errors somehow than to not have it at all.
Thank you for this!

from solid-router.

Yes, it works. Closing the issue then.

from solid-router.