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: runtime error overlay #6274

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

arbassett
Copy link
Contributor

@arbassett arbassett commented Dec 27, 2021

Description

Fixes #2076

To add support for runtime error overlays using vites built in overlay.

it is currently possible to use create-react-app's react-error-overlay in vite but this experience is subpar to how server errors are currently handled

Additional context

This works as a base impetration but i would love to get some feedback on potential addition features and unknowns.

  • Collapsing React's error stack
    • potentially the same for vue. i've never worked with vue before so I'm unfamiliar to how errors are handled
    • Should we provide a api for plugins to extend this so we don't have to handle errors for every possible framework ie import.meta.error
  • How should multiple errors be handled
    • Should we show multiple overlays at the same time an require multiple clicks from the user
    • Should be collect multiple errors and provide a button to toggle between them
  • Should the user be able to follow the stack trace down with sources
  • Should users be able to view the error in source and compiled
vite.runtime.overlay.draft.mp4

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@arbassett
Copy link
Contributor Author

arbassett commented Dec 27, 2021

I'm expecting some tests to break

got them fixed

@Shinigami92 Shinigami92 added the p2-nice-to-have Not breaking anything but nice to have (priority) label Dec 28, 2021
@arbassett
Copy link
Contributor Author

arbassett commented Dec 30, 2021

updated demo showing off promise rejection and more test cases

vite.runtime.overlay.draft.v2.mp4

as a fun aside I was enjoying having a runtime overlay while developing the overlay.. anytime I broke something the overlay would fire and show me what i broke,

Comment on lines +15 to +18
const RE_CHROME_STACKTRACE =
/^ {4}at (?:(.+?)\s+)?\(?(.+?)(?::(\d+))?(?::(\d+))?\)?$/
const RE_FIREFOX_STACKTRACE =
/^(?:(?:(^|.+?)@))\(?(.+?)(?::(\d+))?(?::(\d+))?\)?$/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These feel a bit messy to me but they work. if someone wants to take a crack at making this cleaner i would appreciate it

Comment on lines +161 to +166
if (!fileRE.test(text)) {
el.textContent = text
return
} else {
fileRE.lastIndex = 0
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was added to support none link stack frames like a non-error was thrown please check your browser's devtools for more information

this kinda feels hacky. would love any suggestions on how this should be handled

@arbassett arbassett marked this pull request as ready for review January 4, 2022 20:27
}
}

// TODO: add support for url source maps
Copy link
Contributor Author

@arbassett arbassett Jan 4, 2022

Choose a reason for hiding this comment

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

Does vite even emit url sourcemaps in dev?

@@ -40,14 +40,16 @@ const clientConfig = {
input: path.resolve(__dirname, 'src/client/client.ts'),
external: ['./env', '@vite/env'],
plugins: [
nodeResolve(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Opening the door for dependencies (bundling) in client.mjs feels a bit risky.
Simply bundling source-map makes client.mjs grow from 17k to 127k.

@@ -0,0 +1,266 @@
import type { RawSourceMap } from 'source-map'
import { SourceMapConsumer } from 'source-map'
Copy link
Contributor

Choose a reason for hiding this comment

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

Using source-map in browser seems a bit risky, 0.6 works fine but 0.7 introduced a wasm dependency, and 0.8 (beta) introduces a 300k whatwg-url one.

Copy link
Contributor

Choose a reason for hiding this comment

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

this method could probably be used when we have moved over to source-map-js

sourceMapConsumer?: SourceMapConsumer
baseSource: string
}> => {
const source = await (await fetch(fileName)).text()
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty clever way to get the source map for the file

Though if we consider that we're going to fetch something from vite server anyway, what about sending the error to the server for normalization instead (e.g. through the WS link: socket.send({ type: 'runtime-error', error})) ?
That'd avoid duplicating the work (normalizing errors in vite server & doing it also in vite client) and would also limit the use of dependencies in the client.

My 2c

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea i agree, it would make more sense to use the websocket connection and move the processing over to the server.

@patak-dev
Copy link
Member

Hey @arbassett, sorry for don't get back to you on this one. Now that 2.9 is out, we are starting to prepare for Vite 3.0, and this could be an interesting feature to have in the next Vite major in case you or someone else is interested in keep working on this.

When we talked with the team about this PR, we discussed that:

Another idea, since we have a good client server API maybe we should first check if an external plugin is possible (maybe with some smaller changes or APIs in core). Having it in core could still be the way to go.

Collapsing React's error stack
potentially the same for vue. i've never worked with vue before so I'm unfamiliar to how errors are handled
Should we provide a api for plugins to extend this so we don't have to handle errors for every possible framework ie import.meta.error

This is interesting, maybe we could check how other build tools handle this?

How should multiple errors be handled

  • Should we show multiple overlays at the same time an require multiple clicks from the user
  • Should be collect multiple errors and provide a button to toggle between them

I think we should go with the second option, requiring multiple clicks could be quite bad for some projects.

Should the user be able to follow the stack trace down with sources
Should users be able to view the error in source and compiled

I think we could start without these.

@arbassett
Copy link
Contributor Author

@patak-dev thanks for getting back to me on the questions.

I'm hoping I will have some more time to work on this feature when work dies down.. but if anyone feels like they want to pick it up before I get a chance feel free.

We should move the logic to the server so we keep the client code size small, as proposed by @gluck here: #6274 (comment). We now have a new API in case it is useful https://vitejs.dev/guide/api-plugin.html#client-server-communication, but I think this may be done as an implementation detail.

when I saw the work starting on this I figured its better to wait until its done as its basically what was suggested.

Another idea, since we have a good client server API maybe we should first check if an external plugin is possible (maybe with some smaller changes or APIs in core). Having it in core could still be the way to go.

I think this is the right way to go. at the very least it should allow faster prototyping

There were some worries that people will confuse a runtime error with a Vite error. To avoid this, we should tweak the UI enough to avoid this. I think a different color theme may be enough.

do we have a preference on what colour. I'm not super keen on yellow (warning) or green (everything is good). maybe something thats not used to often like purple/violet

@patak-dev
Copy link
Member

Another idea, since we have a good client server API maybe we should first check if an external plugin is possible (maybe with some smaller changes or APIs in core). Having it in core could still be the way to go.

I think this is the right way to go. at the very least it should allow faster prototyping

Nice, yes, modifying core is a lot more involved. If this could be an external plugin for some time, we may be able to move quicker, get users and feedback, and then at one point we could add it built in if that makes sense.

There were some worries that people will confuse a runtime error with a Vite error. To avoid this, we should tweak the UI enough to avoid this. I think a different color theme may be enough.

do we have a preference on what colour. I'm not super keen on yellow (warning) or green (everything is good). maybe something thats not used to often like purple/violet

Yes, I was also thinking about that tones. We could also add a label to make it more clear. IMO, we also don't need to be constrained by the current overlay design here. Maybe for runtime errors, it makes more sense to have something less intrusive? For a Vite error, we normally want to cover the screen, but for runtime errors, maybe the UI is functional? And we could use temporal toasts that don't block the UI?

image

Just an idea, but we could play with a DX that is more fit to each error type.

@bluwy
Copy link
Member

bluwy commented Jun 16, 2022

+1 for toasts too, otherwise we may get similar request like vercel/next.js#13387

@hichemfantar
Copy link

This is very good work! Any idea when this gets merged?

@Inviz
Copy link

Inviz commented Jan 18, 2023

Is there a way to use it without maintaining forked vite?

@gaurav21r

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature request] Show browser error overlay for runtime errors
10 participants