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

Initial dev server #5317

Merged
merged 5 commits into from
Sep 28, 2018
Merged

Initial dev server #5317

merged 5 commits into from
Sep 28, 2018

Conversation

timneutkens
Copy link
Member

@timneutkens timneutkens commented Sep 27, 2018

Move all parts that rely on this.dev into it's own server that is built on top of the default server.

Copy link
Contributor

@giuseppeg giuseppeg left a 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
Copy link
Contributor

@giuseppeg giuseppeg Sep 28, 2018

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

Copy link
Member Author

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 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed 🙌

@timneutkens timneutkens merged commit 3f47a87 into vercel:canary Sep 28, 2018
@timneutkens timneutkens deleted the add/dev-server branch September 28, 2018 12:05
async prepare () {
await super.prepare()
if (this.hotReloader) {
await this.hotReloader.start()
Copy link
Contributor

@HaNdTriX HaNdTriX Sep 28, 2018

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) ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Great catch! #5320

timneutkens added a commit that referenced this pull request Sep 28, 2018
Remove `if(this.hotReloader)` as it's always guaranteed to be there.

#5317 (comment)
@lock lock bot locked as resolved and limited conversation to collaborators Sep 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants