-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Graceful error recovery in the dev server #5198
Conversation
🦋 Changeset detectedLatest commit: 278bb7a The changes in this PR will be included in the next version bump. 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 |
!preview hmr-recovery |
|
@matthewp No more fixture tests? That's amazing, very excited to give this a thorough review when I can! |
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.
Looks awesome!
Very curious about the test structure here. I suppose packages/astro/test/units/**
is the path of least resistance, but would it make more sense to have unit tests next to the files they are testing? Maybe that's something we can follow-up with.
Edit: Realized this is still a draft, dismissing my approval. Happy to circle back when this is ready!
!preview hmr-recovery |
|
49a3f00
to
cd30ef7
Compare
/** | ||
* The MIT License (MIT) | ||
* Copyright (c) 2018 Andy Wermke | ||
* https://github.com/andywer/typed-emitter/blob/9a139b6fa0ec6b0db6141b5b756b784e4f7ef4e4/LICENSE |
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.
Note that I brought this package internally because when Vite crawls through package.jsons, this one has a main but the main doesn't export anything so Vite warns about it not being a real ESM module. This is only a (small) types package, so brought it in.
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.
Looks great! With the amount of refactor for ModuleLoader
, I might've gave up and mock fs directly instead 😄 But it'll be interesting to see how this plays out too.
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.
Looks straightforward. Nice refactors!
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.
Nice! Big fan of the refactor to a loader
interface for module resolution and the breakup of the dev server logic, makes for a much cleaner code base 🚀
meta?: Record<string, any>; | ||
} | ||
|
||
export function createLoader(overrides: Partial<ModuleLoader>): ModuleLoader { |
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.
Very nice, I really like how the loader interface cleaned up the code flow here 👍
Move dev-container to dev Update for the lockfile Invalidate modules in an error state Test invalidation of broken modules Remove unused error state Normalize for windows try a larger timeout Fixes build just for testing more testing Keep it posix fully posix
b62e062
to
278bb7a
Compare
Changes
Issues affected
Testing
core/dev
, which is meant to encompass all of what dev does. This essentially makes the dev server unit-testable without needing to start the actual dev server and send HTTP requests. You can also define .astro (or other modules) in a virtual filesystem, so no more fixture tests.Docs
N/A