chore: shrink error messages shipped to client #11551
Conversation
🦋 Changeset detectedLatest commit: d7fcbf2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
benmccann
force-pushed
the
smaller-error-msgs
branch
from
8fccaa1 to
ebd1c52
Compare
packages/kit/src/utils/routing.js
Outdated
| @@ -65,7 +67,9 @@ export function parse_route_id(id) { | |||
| const match = param_pattern.exec(content); | |||
| if (!match) { | |||
There was a problem hiding this comment.
Could remove this whole block on the client side I think, the manifest generation would fail on this so it never gets here on the client.
There was a problem hiding this comment.
Looks like you removed it altogether,
I don't remember the specifics but iirc the error during manifest generation comes from the same place, so it can only be removed in the client bundle.
|
I think we should probably have a more principled approach to this problem — rather than making messages shorter (read: less helpful) in production, what if we did something similar to what React does and printed errors/warnings with codes that pointed to documentation? It could be as simple as this |
|
We definitly should have such an approach to our error messaging in the mid term |
packages/kit/src/utils/routing.js
Outdated
| // Skip invalid params. | ||
| // Params and matcher names can only have underscores and alphanumeric characters. | ||
| // We'll never hit this because mnifest generation would have failed. | ||
| if (!match) return; |
There was a problem hiding this comment.
if we never hit it, why do we have this line at all? surely a type assertion would be sufficient?
we should probably be separating these changes into different PRs — this isn't related to error messages (which i think we should solve properly rather than with piecemeal changes), and the disable_hash stuff is a behaviour change that probably warrants discussion
There was a problem hiding this comment.
I like your idea about how to handle error messages. If we had that, I don't think we should change this though - better to ship no error message than a small unreachable one
Yes, I like that idea. If we do that, I think I'd still want all the changes here:
I could clarify the changeset message to say "remove and clarify error messages" or something along those lines. I'd be happy to break this into multiple PRs, but size-wise this is a nice size PR and we'd end up with three pretty small PRs if I did each of those changes separately. I do like the idea of having an errors page, but if we're wanting to split PRs, I don't think I'd mix it in with these changes and it would be better to do it separately. |
No description provided.