fix: show error if vite client cannot be loaded #17419
Conversation
|
|
benmccann
force-pushed
the
benmccann-patch-1
branch
4 times, most recently
from
9cb1c1c
to
609d437
Compare
benmccann
added
the
p3-minor-bug
|
I feel we could improve the DX by having the error overlay support a "standalone" mode or "non-dismissable" mode? That way the error overlay is non-dismissable (as there's no reason so), and if they accidentally clicked outside, they won't get a lousier UI. I also think we should only render the fallback error if the error overlay fail to load? We should also add an additional message that the error overlay failed to load. |
|
Those sound like reasonable ideas, but are all more complicated to implement and are just UX niceties. Maybe we could move ahead with this to fix the core issue of having a blank page and then file an issue for the followup ideas |
|
I don't think the issue is urgent enough that we should go ahead with this middleground, since the error can be viewed from the terminal too? So I'm leaning on doing a proper fix instead 😬 |
benmccann
force-pushed
the
benmccann-patch-1
branch
from
609d437
to
1f774b2
Compare
I made both of these changes
This part really feels separate to me. The current PR is purely an improvement over the current state. In the cases where there previously was a blank page we're now showing an error and in the cases where the overlay was rendering it's exactly the same. I do like the idea of a non-dismissable mode, but think we could handle that in a separate PR |
|
Thanks! Yeah fair we can handle that separately 👍 |
| try { | ||
| const { ErrorOverlay } = await import('/@vite/client') | ||
| document.body.appendChild(new ErrorOverlay(${JSON.stringify( | ||
| prepareError(err), | ||
| ).replace(/</g, '\\u003c')})) | ||
| } catch { | ||
| document.innerHTML = \`<h1>Internal Server Error</h1><h2>Error overlay failed to load</h2><pre>${err.stack?.replaceAll('<', '<').replaceAll('$', '$')}</pre>\` | ||
| } |
There was a problem hiding this comment.
Question: Could we have the JSON.stringify(prepareError(err)).replace(...) kept in a variable outside the try-block, and re-use it here too? That way we have a consistent formatting of the error (e.g. stacktrace is ansi-stripped), and we don't need to escape here differently.
There was a problem hiding this comment.
I'd originally tried doing the same .replace(/</g, '\\u003c'), but it wasn't working. I don't fully understand why it needs different escaping here or the difference between them. In any case, we would still need an extra escaping of $ in this second use because it's being used as a template string, which is different than the first use
Description
fixes #17418
Ensure that we show an error if
/@vite/clientcannot be loaded. Prior to this PR the error handler worked only with post middlewares. Now it works with all middlewares