-
Notifications
You must be signed in to change notification settings - Fork 27.4k
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
Initial dev server #5317
Initial dev server #5317
Conversation
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.
not familiar with the codebase but the idea sounds good
server/index.js
Outdated
@@ -23,7 +23,6 @@ import pkg from '../../package' | |||
export default class Server { | |||
constructor ({ dir = '.', dev = false, staticMarkup = false, quiet = false, conf = null } = {}) { | |||
this.dir = resolve(dir) | |||
this.dev = dev | |||
this.quiet = quiet | |||
this.router = new Router() | |||
const phase = dev ? PHASE_DEVELOPMENT_SERVER : PHASE_PRODUCTION_SERVER |
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.
you could probably get rid of dev
if you use the constructor only to define instance configuration and then add a init
method to the class which you'd call explicitly. This way the dev server could do the extra work inside its constructor
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.
Yeah, this is just the first wave of changes, I'm going to progressively move more code to the dev server 👍
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.
Fixed 🙌
async prepare () { | ||
await super.prepare() | ||
if (this.hotReloader) { | ||
await this.hotReloader.start() |
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.
We can now omit these checks.
if (this.hotReloader) ...
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 catch! #5320
Remove `if(this.hotReloader)` as it's always guaranteed to be there. #5317 (comment)
Move all parts that rely on
this.dev
into it's own server that is built on top of the default server.