-
Notifications
You must be signed in to change notification settings - Fork 427
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(core): specific error when dev server is stopped #7476
Conversation
…dev server stop error
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
No changes to documentation |
Component Testing Report Updated Sep 30, 2024 2:25 PM (UTC) ✅ All Tests Passed -- expand for details
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really cool feature 🙌. Left some thoughts about non-Vite support and minimising connections and custom code.
…is only for default vite
useEffect(() => { | ||
// no early return to optimize tree-shaking | ||
if (isViteServer(serverHot)) { | ||
serverHot.on('vite:ws:disconnect', markDevServerStopped) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@juice49 using import.meta.hot.on
. In our version of vite (4.x) there is no .off
, however there is in 5.x. Don't think it's a big issue here, since restarting the server will trigger a refresh on the client. But does raise a question around us upgrading perhaps... one for another time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting omission from the v4 API 🤔. Do you have any concerns around creating multiple subscriptions that are never cleaned up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not concerned around multiple subscriptions since starting the server again causing a remounting of the entire app, clearing all existing listeners
Added to transform |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, @jordanl17!
@juice49 right you are, I'd forgotten to commit the final commit to rename a few things which was causing this. Note however that navigating away from the initial 'dev server stopped' error will still show the dynamic import error. It would be possible to rectify this, but I can't think of how besides complicating the current setup, which doesn't feel appropriate given this is meant to just be an incremental dev experience improvement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brilliant! Looks great to me 🥳.
(Noted and agree with you on the dynamic import error; just wanted to mention it in case it provided useful context).
Description
Currently when the dev server is stopped, when the next error is thrown (for example a new lazy bundle is fetched), then an error
failed to fetch dynamic import
is thrown and displayed. It doesn't make it clear that this has happened because dev server has gone away.Now, immediately once the dev server is stopped, the studio will show an error boundary stating such.
What to review
DevServerStopped.tsx
DevServerStopped
is lazy imported to avoid adding this to the bundle when not in development modeStudioLayout.tsx
throwing the dev server stopped error which is causing in studio error boundary, from where the specific screen is shownTesting
Notes for release