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

Clean up dev-server options #1175

Merged
merged 1 commit into from Oct 15, 2018
Merged

Clean up dev-server options #1175

merged 1 commit into from Oct 15, 2018

Conversation

edmorley
Copy link
Member

@edmorley edmorley commented Oct 13, 2018

  • (BREAKING) Stops setting host / public, since they are only needed in a minority of use-cases (such as running inside a Docker container or having a proxy in front of webpack-dev-server), and the current implementation for them in @neutrinojs/dev-server is buggy.
  • (BREAKING) Stops supporting the @neutrinojs/web shorthand of setting devServer.proxy to a string, since it could only be used if the options it set were exactly what were required - which was often not the case given that proxy configuration is very project-specific.
  • Stops setting a host header on responses to the client, since it makes no sense (particularly it was set to the public host). It's possible that this was set thinking they would be set for requests to a proxy target, but that's not the case (and for that, changeOrigin: true would be more appropriate anyway).
  • Changes output.publicPath from './' to '' since the latter is equivalent (and is also the webpack default), however it correctly allows webpack-dev-server to fall back to '/' when needed:
    https://github.com/webpack/webpack-dev-server/blob/v3.1.9/lib/Server.js#L176
  • Stops setting devServer.publicPath explicitly since it inherits from output.publicPath so should not normally need to be overridden:
    https://github.com/webpack/webpack-dev-server/blob/v3.1.9/bin/webpack-dev-server.js#L142-L152
  • Stops setting https: false, since that's the default.

Fixes #1169.

@edmorley edmorley added this to the v9 milestone Oct 13, 2018
@edmorley edmorley self-assigned this Oct 13, 2018
@@ -112,9 +89,6 @@ By default this middleware will start a development server with Hot Module Repla
`http://localhost:5000`. To enable HMR with your application, read the documentation of corresponding Neutrino
preset or middleware.

It is recommended to call this middleware only in development mode when `process.env.NODE_ENV === 'development'`.
Copy link
Member Author

@edmorley edmorley Oct 13, 2018

Choose a reason for hiding this comment

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

Now that the @neutrinojs/dev-server preset only sets the devServer option, there's no real reason why the middleware shouldn't always be used.

@edmorley edmorley requested review from eliperelman and timkelty Oct 14, 2018
* (BREAKING) Stops setting `host` / `public`, since they are only needed
  in a minority of use-cases (such as running inside a Docker container
  or having a proxy in front of webpack-dev-server), and the current
  implementation for them in `@neutrinojs/dev-server` is buggy.
* (BREAKING) Stops supporting the `@neutrinojs/web` shorthand of setting
  `devServer.proxy` to a string, since it could only be used if the
  options it set were exactly what were required - which was often not
  the case given that proxy configuration is very project-specific.
* Stops setting a `host` header on *responses* to the client, since
  it makes no sense (particularly it was set to the public host).
  It's possible that this was set thinking they would be set for
  *requests* to a proxy target, but that's not the case (and for that,
  `changeOrigin: true` would be more appropriate anyway).
* Changes `output.publicPath` from `'./'` to `''` since the latter is
  equivalent (and is also the webpack default), however it correctly
  allows webpack-dev-server to fall back to `'/'` when needed:
  https://github.com/webpack/webpack-dev-server/blob/v3.1.9/lib/Server.js#L176
* Stops setting `devServer.publicPath` explicitly since it inherits from
  `output.publicPath` so should not normally need to be overridden:
  https://github.com/webpack/webpack-dev-server/blob/v3.1.9/bin/webpack-dev-server.js#L142-L152
* Stops setting `https: false`, since that's the default.

Fixes #1169.
@edmorley
Copy link
Member Author

@edmorley edmorley commented Oct 14, 2018

* Changes `output.publicPath` from `'./'` to `''` since the latter is equivalent (and is also the webpack default), however it correctly allows webpack-dev-server to fall back to `'/'` when needed

Note we may be changing this again in #1171, however I wanted to get the ~"no-op" cleanup out of the way first.

}
};
}

Copy link
Contributor

@timkelty timkelty Oct 15, 2018

Choose a reason for hiding this comment

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

Should we include the headers bit in our example? If not here, perhaps a page in the docs for "Using Neutrino with Existing SSR Architecture" would make sense.

Copy link
Member Author

@edmorley edmorley Oct 15, 2018

Choose a reason for hiding this comment

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

I think we could easily write several pages on how to set up SSR. Multiple other parts of the webpack config needs adjusting, eg I'm pretty sure html-webpack-plugin doesn't fully work with it out of the box and needs some experimental options enabled.

I think this example is perhaps best aimed at the more common case of "I have a standard SPA but it uses a backend and I want to proxy to it in development". (Not that it precludes adding a more advanced SSR guide somewhere else in the future)

Copy link
Member Author

@edmorley edmorley Oct 15, 2018

Choose a reason for hiding this comment

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

Also, perhaps an SSR guide is best suited as an extra page in webpack's docs? Ultimately Neutrino is not meant to be a replacement for all of webpack/...'s documentation, but a very thin layer of abstraction that assists with configuration generation, sets some sensible defaults, and that makes it easy to mentally map between a webpack guide and how to add that configuration via .neutrinorc.js or via a custom Neutrino preset.

Copy link
Contributor

@timkelty timkelty Oct 15, 2018

Choose a reason for hiding this comment

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

#1177 :)

Copy link
Member Author

@edmorley edmorley Oct 15, 2018

Choose a reason for hiding this comment

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

I think we could easily write several pages on how to set up SSR

I underestimated how many pages...!
https://ssr.vuejs.org/

Copy link
Contributor

@timkelty timkelty Oct 15, 2018

Choose a reason for hiding this comment

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

Also: it's really 2 different things if you're talking about SSR with React/Vue vs. using webpack/neutrino for "assets", and something else for SSR of html (PHP, Ruby, etc)

Copy link
Member Author

@edmorley edmorley Oct 15, 2018

Choose a reason for hiding this comment

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

Yeah true

@edmorley edmorley mentioned this pull request Oct 15, 2018
Copy link
Member

@eliperelman eliperelman left a comment

Looks fantastic.

@edmorley edmorley merged commit 64a373b into neutrinojs:master Oct 15, 2018
2 checks passed
@edmorley edmorley deleted the devserver-cleanup branch Oct 15, 2018
@constgen
Copy link
Contributor

@constgen constgen commented Nov 3, 2018

@edmorley I believe the story of these settings is here #187

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants