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

feat: provide better error messages in DEV #11526

Merged
merged 18 commits into from
May 10, 2024
Merged

feat: provide better error messages in DEV #11526

merged 18 commits into from
May 10, 2024

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented May 9, 2024

Today, Svelte 5's error message is a bit lacking. This PR aims to make the error messages much better in DEV. Previously, you'd get given the error message and some stack that likely meant nothing. With this PR, you now get the component
stack too along with a clearer message. At also lets you know if the error occurred in different types of effect.

  • We now name the effect functions, so they're clearer in the stack trace as to what they are
  • We filter out Svelte internal functions from stack traces, so it makes it easier to see what is likely causing the issue
  • We provide the component stack up until when the error occurred so you can what components we used in the lead up the error
  • We now avoid retrying hydration or showing a mismatch for user errors. We keep the hydration mismatch warning for actual mismatches.
  • Fixed a bunch of subtle things I encountered when working on this.

Copy link

changeset-bot bot commented May 9, 2024

🦋 Changeset detected

Latest commit: 5971159

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

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

@Rich-Harris
Copy link
Member

Rich-Harris commented May 10, 2024

It would be nice if we could just replace the new error stack with the original rather than including the original in the message, since stack traces are given special treatment by devtools (e.g. respecting sourcemaps, excluding things as specified by x_google_ignoreList). The exact treatment differs from browser to browser, but this is what I'm seeing in the playground in Firefox — it's a useless wall of text:


image

If I do new_error.stack = error.stack instead of adding the stack to component_stack_string then I get this instead, which is much better:


image

Unfortunately that kills the error message in Chrome, so there's more finessing required.

Then again: if we have a working stack trace, do we need the component tree as well? What additional information does it provide? (Also, it's currently misidentifying the component responsible for the error, for me — it's blaming the component's parent.) I'm not clear to me what we gain by calling handle_error.

@trueadm
Copy link
Contributor Author

trueadm commented May 10, 2024

@Rich-Harris I addressed the issues you mentioned. This PR wasn't finished yesterday when you reviewed it and it had some bugs and missing pieces still yet to do. The stack traces now properly work with sourcemaps.

Co-authored-by: Rich Harris <rich.harris@vercel.com>
Co-authored-by: Rich Harris <rich.harris@vercel.com>
Comment on lines 273 to 279
if (effect_name) {
// If we have an effect name, it means it happened in the template, $effect or $effect.pre.
Object.defineProperty(error, '__skip_hydration_retry', {
value: true,
enumerable: false
});
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this heuristic feels brittle. if an error occurs during component initialisation, effect_name is empty, and skip_retry will be false. AFAICT there's only one way a hydration mismatch error will be thrown, and that's e.hydration_missing_marker_close() in hydration.js. What if we just check whether the caught error had that code, rather than this __skip_hydration_retry stuff?

Copy link
Contributor Author

@trueadm trueadm May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. although I can still see effect_name work on init as we set the function name before calling the function now. Although I wonder if the DOM traversal logic might encounter it too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this is the only way this can happen? What if the DOM was modified by some extension and a bogus element was inserted, so the walking mechanism fails and a delete everything + remount would fix it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete everything + remount would fix it?

That's not really a fix though, remounting everything is a last resort and they should really fix their problem, no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would they be able to fix if a browser extension messes with the dom?

Copy link
Contributor Author

@trueadm trueadm May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad browser extensions break every framework, regardless of us trying to fix it during hydration. The extension will just use a MutationObserver to try and re-apply their changes after – such as Google Translate or ad blockers. This has plagued every library/framework since the dawn of time too. I remember having issues with this and jQuery + ad blocker back in the day!

This was particularly complex with Lexical because of contentEditable. The only thing you can do is let people complain and then point them to turning off their extension. You can go further and add a MutationObserved in DEV that warns on mutations outside of Svelte, but that seems like a step too far for us to go.

We should never optimise for bad web extensions, it's a losing battle. What we can maybe do is add line numbers to the DOM traversal logic and supply the expected element name:

var node = $.sibling(div, [23, 0, 'span']);
var text = $.child(div, [24, 0, 'text']);

Or maybe we can reference the line numbers from the template somehow. @Rich-Harris any ideas?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like overkill to me (and what if the element was wrong but had the same name? we'd still be screwed)

@trueadm trueadm merged commit fcc72ae into main May 10, 2024
8 checks passed
@trueadm trueadm deleted the better-errors branch May 10, 2024 19:26
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.

None yet

3 participants