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: display error overlay #607

Merged
merged 13 commits into from
Mar 9, 2025
Merged

feat: display error overlay #607

merged 13 commits into from
Mar 9, 2025

Conversation

Valerioageno
Copy link
Member

@Valerioageno Valerioageno commented Feb 26, 2025

Checklist

Related issue

Fixes #592

Overview

Implementation detail

The dialog is a vite internal web component that gets automatically triggered by its WebSocket connection.

In order to make this feature working I had to "hack" a little bit the current implementation

  1. Since most of the compilation errors causes a failed SSR I created a fallback_html file that gets injected in .tuono/index.html that simply renders the project client side and open the connection to the vite server.
  2. There is no a straighforward way to override the defaul vite error overlay. I followed the same implementation as astro (as suggested by one of the vite maintainers HMR overlay customization vitejs/vite#19552)

Implementation split

  1. Build index.html from tuono CLI
  2. Build error-overlay.ts component in the vite build
  3. Add e2e test for the overlay (this caused issues, will do it in a separate PR)

Screenshot

For this iteration the overlay is just a clone of the vite one. In the future we might need to refactor the UI in order to make it more tuono style like. No changes to the current PR should be required

Screenshot 2025-03-04 182324

@Valerioageno Valerioageno self-assigned this Feb 26, 2025
@github-actions github-actions bot added typescript Requires typescript knowledge rust Requires rust knowledge labels Feb 26, 2025
@Valerioageno Valerioageno force-pushed the add-vite-error-boundary branch 10 times, most recently from ef36f03 to 1c0c740 Compare March 5, 2025 18:09
@Valerioageno Valerioageno force-pushed the add-vite-error-boundary branch 4 times, most recently from d7c4569 to 927cb43 Compare March 6, 2025 11:42
@github-actions github-actions bot added repo maintenance linting, fs organization, that sort of stuff CI/CD labels Mar 6, 2025
@Valerioageno Valerioageno force-pushed the add-vite-error-boundary branch from ced09cc to c18590a Compare March 6, 2025 16:04
@github-actions github-actions bot removed the CI/CD label Mar 6, 2025
@github-actions github-actions bot removed the repo maintenance linting, fs organization, that sort of stuff label Mar 6, 2025
@Valerioageno Valerioageno marked this pull request as ready for review March 6, 2025 16:09
@Valerioageno Valerioageno requested a review from jacobhq March 6, 2025 16:09
Copy link
Member

@jacobhq jacobhq left a comment

Choose a reason for hiding this comment

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

Overall this looks pretty good. I don't understand the removal of the class fields on the ts side. Would you be able to tell me more about that?

@Valerioageno
Copy link
Member Author

@marcalexiei @jacobhq Updated! Let me know if I can merge

marcalexiei
marcalexiei previously approved these changes Mar 7, 2025
@marcalexiei marcalexiei requested a review from jacobhq March 7, 2025 18:13
Copy link
Member

@jacobhq jacobhq left a comment

Choose a reason for hiding this comment

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

All good from me. Nice work!

@Valerioageno Valerioageno merged commit 77c902a into main Mar 9, 2025
29 checks passed
@Valerioageno Valerioageno deleted the add-vite-error-boundary branch March 9, 2025 10:32
@marcalexiei marcalexiei mentioned this pull request Mar 9, 2025
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rust Requires rust knowledge typescript Requires typescript knowledge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create ErrorOverlay component
3 participants