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: read error from custom element attributes in vite-error-overlay #5686

Closed
wants to merge 4 commits into from

Conversation

frandiox
Copy link
Contributor

Description

Currently, SSR frameworks based on Vite that want to keep the same error overlay experience on SSR errors need to either:

  • Use vite.ws.send({ type : 'error', err }) (or next(err)) wrapped in an arbitrary timeout to wait until the browser has downloaded the main Vite code that registers vite-error-overlay custom element and handles the WebSocket.
  • Send some hacky code back to the browser that finds vite-error-overlay in the CE registry and calls it.

This PR enables vite-error-overlay custom element to read the error information from attributes if nothing is passed to the constructor. This allows creating error overlays declaratively as HTML: <vite-error-overlay message="..." stack="..."></vite-error-overlay>, which can be sent as part of the SSR response.

Additional context

@patak-js Is this something the Vite team is comfortable with?


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.

@frandiox
Copy link
Contributor Author

@patak-dev did you get the chance to ask the team about this?

@patak-dev patak-dev added the p2-nice-to-have Not breaking anything but nice to have (priority) label Dec 20, 2021
@patak-dev patak-dev added this to the 2.8 milestone Dec 20, 2021
@bluwy
Copy link
Member

bluwy commented Dec 23, 2021

#6184 is doing something similar to tackle this issue too. It's exporting the ErrorOverlay class from /@vite/client and displaying it in a script, so it's more imperative in the markup-side (and possibly more flexible). Do we want to follow along that PR's approach? or should we stick with this PR's.

Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

Just my approval for the code, but please read @bluwy's comment

@frandiox
Copy link
Contributor Author

@bluwy Yeah, that's not very different from what we're currently doing.
I thought making it declarative as Custom Element attributes was simpler, that's why I made this PR. It's ultimately up to you guys 👍

@bluwy
Copy link
Member

bluwy commented Dec 28, 2021

Looks like #6184 got merged. I think we should follow that to have one way of doing things, hopefully it'll make the code less hacky 😬

@patak-dev
Copy link
Member

@frandiox @bluwy should we wait then to merge this PR?

@frandiox
Copy link
Contributor Author

@patak-dev Looks like we are moving towards using ErrorOverlay in an imperative way instead of declarative so perhaps we can just close this PR?
I'm not sure if you need to double check with others in the team or it is already decided but feel free to postpone this PR or close it.

@patak-dev
Copy link
Member

Could you explain a bit more how #6184 affected this PR (that had approval from the team)? I thought that #6184 was only correcting the case when there is malformed HTML. Are your use cases covered without this PR? Or do you intend to add support for them with another approach?

As a note, there is now work to use the error overlay for runtime errors here #6274

@bluwy
Copy link
Member

bluwy commented Dec 30, 2021

@patak-dev In #6184, it made a change to export the ErrorOverlay class from /@vite/client. It is a custom element and has been used to programatically create the error overlay (example). With this, SSR frameworks can now do the same thing to show an overlay. This PR proposes another way for us to show the error overlay, which means there would be two ways of doing the same thing. Hence it was an either-or situation here, and #6184 got merged instead.

@patak-dev
Copy link
Member

We could mark ErrorOverlay as internal if we think the API is not the best at this point, that wasn't the intention of the PR. @frandiox or @bluwy could you review that the current API is a good fit for frameworks to use and maybe PR docs for this feature? We can check it next week in the team meeting to discuss what should be the recommended approach.

@bluwy
Copy link
Member

bluwy commented Dec 30, 2021

I currently leaning towards the current (imperative) API as we don't have to maintain a separate source of errors retrieved via attributes. But SvelteKit hasn't used the error overlay element before, so I can't speak of how ergonomic it will be in practice. I guess it's @frandiox's call here since they have more experience with this.

@patak-dev patak-dev removed this from the 2.8 milestone Jan 2, 2022
@bluwy
Copy link
Member

bluwy commented Feb 26, 2023

Closing this for now as it has been stale. We should stick with the imperative way at the meantime.

@bluwy bluwy closed this Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on hold 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.

5 participants