solidjs
[SSR] Seroval can't serialize axios error in createResource and breaks the app
Describe the bug
When createResource throws some specific error (in this case it's axios error) that seroval can't handle, the app gets broken with the following error message: "Error: Cannot serialize function without reference ID."
Your Example Website or App
https://stackblitz.com/edit/solid-ssr-vite-ispxqp?file=src/routes/index.tsx
Steps to Reproduce the Bug or Issue
- This bugs can only be reproduced when using SSR.
- Go to the terminal and observe the described error message.
Expected behavior
createResource should be able to handle any error no matter the type and what place this error was thrown from.
If the error can't be serialized for some reason, createResource shouldn't break the whole app and probably fallback to something meaningful (default error + message from the original error?).
Screenshots or Videos
No response
Platform
- OS: macOS 12.6.8
- Browser: Chrome
- Version: 118
Additional context
No response
Seems like axios.get's error has a property (or within it) that contains a function (moreover, the error object itself is a complex object and has values that seroval cannot serialize, I tried inspecting it myself), this would require the user to intercept the error so that seroval is able to serialize it.
Example:
const [data] = createResource(async () => {
try {
return await axios.get('https://some-random-nonexistent-url.org');
} catch (error) {
return new Error(error.message);
}
});
@lxsmnsyc Yeah that makes sense but I think this inconsistent behavior might not be the best developer experience and sometimes you can't really predict what error would be thrown from a library and whether seroval could handle that error or just fail. Just imagine a case where some third party library throws a simple error that seroval can handle but the other day they decided to add some useful stuff in there that seroval can't handle anymore and after update your app is crashed because you didn't predict this beforehand and don't handle your error. Sounds not that great, right?
In additional, createResource is supposed to handle errors on its own (that's why we have error property) so you can catch them later with <ErrorBoundary /> somewhere in your code. So it should handle any error in such case, not just a subset of them.
supposed to handle errors on its own
It does however allowed errors in createResource has the same restriction as the allowed values it can resolve into. The thing is, SSR serializes the result of createResource, and it cannot serialize any value that it can't handle. It breaks correctness on both server and client.
allowed errors in createResource has the same restriction as the allowed values it can resolve into.
Yep, I understand that but my point here is that the current developer experience can definitely be improved, at the very least we can provide more useful error with some suggestions if we can't do any better here. However, I think we still have a few other options to improve the behavior: throwing a custom useful error instead of an original one (and stripping unserializable fields) or maybe forcing a user to handle errors on their own, or maybe something else. Anyway, we have a work to do here because the current error could be confusing for many people.
Okay I guess our common ground here is for Solid to intercept Seroval's errors and give a Solid-related warning.
@lxsmnsyc At least yeah, but it would be much more convenient to have something more high-level so you don't have to re-throw a common error because you don't even know beforehand whether an original error could be serialized or not.
@mattersj whether something is serializable or not is going to be documented in the future for sure, but we can't really change the original intent of the user as it introduces more inconsistencies.
@lxsmnsyc I meant that you can't tell if your error has some unserializable data if you haven't analyzed it on purpose (which nobody does) and even if you know that functions can't be serialized you still couldn't tell if some third party library has functions in their errors which is a bummer. But I got your point overall, would like to see what @ryansolid thinks about that.
Ok. Looking at what is a reasonable resolution.
What I'm thinking:
If the value you can't be serialized whether it be promise success or error, the fact it can't serialize has to trump as an error as we can't do anything with the original value anyway. Like it might be an error. It might not be.
Ideally this error should not crash the server.. if it could be caught in the resource as well then we can just have the normal error boundary behavior.
Other considerations:
More human readable error. If we go that way it probably won't be much clearer. But it is probably doable.
Should non-serializable fields cause a serialization failure or just be skipped. I can tell being skipped is what is desired here, but in the general case that is dangerous because what you put in is not what you get out.
Let me look and see what we can feasibly do here. Also since I think the other 2 bugs we have open right now are accenting these. It might be interesting to see what this looks like in isolation once those are fixed.
Should non-serializable fields cause a serialization failure or just be skipped. I can tell being skipped is what is desired here, but in the general case that is dangerous because what you put in is not what you get out.
This is what I'm against, and given Seroval's current structure, this is highly unlikely to happen.
Yeah I agree.. and because of the handoff to the serializer which resolves async there is no reasonable way to contain this within things like error boundaries. The resource passes its promise along and then its done. Seroval would need to hand its promises back to us to do anything like that. I don't like that it crashes the server though. An API maybe were we could pass errors out of the server render function is all I can think of at the moment.
We added an onError to the server render function to collect seroval errors in 1.8.4 but I think it is interesting question on what should happen to the request in these scenarios. We can't really handle this at error boundaries as it is detached, but maybe we could send a 500 response back? I don't know if we should default that or just pass that power completely to the consumer. But at least for now we aren't causing the server to crash to my knowledge.
I haven't had a chance to test 1.8.4 in SolidStart since it still uses Solid 1.8.3 so the server still gets crashed, but I think it'd be great if we could introduce a user-friendly 500 error in these cases explaining why things went wrong and pointing out the exact spot this happened. I'd say Next.js does a good job here:
Yeah we won't be able to do that easily in that we pass on the call site and the reason for that is that the whole system is a progressive serialization machine. Like we have no reference to where the serialization started..
It's like:
// inside your resource, inside some data function, inside some route
promise = yourFn();
serialize(promise);
promise.then(v => {
setResourceSignal(v);
triggerSuspenseStuff()
})
// somewhere else
function serialize(promise) {
promise.then(v => {
serializeValue(v); //error happens here
checkHowTheStreamIsGoing();
})
}
I wonder How Next handles this in the App Directory because serialization I imagine is similarly detached. It was all fine when they just called getServerSideProps at a location and handled the error there.
I guess the only thing I can think of is if the serializer returned its own promise that it generated and we listen to that, but I don't know how feasible that is, it also begs a bit of a question on timing.