-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add HMR to the dev server #641
Conversation
@@ -14,7 +15,8 @@ module.exports = merge(sharedConfig, { | |||
devServer: { | |||
clientLogLevel: 'none', | |||
host: settings.dev_server.host, | |||
port: settings.dev_server.port, | |||
port: settings.dev_server.port |
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.
Missing trailing comma ;)
@@ -14,7 +15,8 @@ module.exports = merge(sharedConfig, { | |||
devServer: { | |||
clientLogLevel: 'none', | |||
host: settings.dev_server.host, | |||
port: settings.dev_server.port, | |||
port: settings.dev_server.port | |||
hmr: settings.dev_server.hmr, |
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.
Option name is hot: settings.dev_server.hmr
I will push some updates for sass.js since the styles won't be extracted when HMR is on 👍 and then we should be good to go |
@@ -42,7 +45,9 @@ def javascript_pack_tag(*names, **options) | |||
# <%= stylesheet_pack_tag 'calendar', 'data-turbolinks-track': 'reload' %> # => | |||
# <link rel="stylesheet" media="screen" href="/packs/calendar-1016838bab065ae1e122.css" data-turbolinks-track="reload" /> | |||
def stylesheet_pack_tag(*names, **options) | |||
stylesheet_link_tag(*sources_from_pack_manifest(names, type: :stylesheet), **options) | |||
unless Webpacker.dev_server.running? && Webpacker.dev_server.hot_module_replacing? |
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.
This is not correct in that React on Rails users will build static assets for test runs while the dev server is possibly running for development. Thus, we'll skip this tag when we need it for tests.
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.
@justin808 What are the implications? I guess dev server can be used too for serving assets in test environment and don't think that will affect any behaviour. Obviously, if someone wants to statically build and test they can turn off the dev-server and use watcher or on-demand compilation. With dev-server proxy usage is more transparent and consistent so the output will be same in views regardless of compiler used.
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 fixed it such that Webpacker.dev_server.running? will return false in testing by moving the dev_server configuration to the development env only in webpacker.yml. So now the tag will appear in test mode.
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.
@gauravtiwari wrote:
What are the implications?
Since we have a different test setup in webpack for test compared to dev (we want more logging in dev), I'd feel uncomfortable that some of the setup is coming from a different webpack config.
@@ -42,7 +45,9 @@ def javascript_pack_tag(*names, **options) | |||
# <%= stylesheet_pack_tag 'calendar', 'data-turbolinks-track': 'reload' %> # => | |||
# <link rel="stylesheet" media="screen" href="/packs/calendar-1016838bab065ae1e122.css" data-turbolinks-track="reload" /> | |||
def stylesheet_pack_tag(*names, **options) | |||
stylesheet_link_tag(*sources_from_pack_manifest(names, type: :stylesheet), **options) |
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 need to update the above docs to indicate what happens in HMR, as well as for the possibility that the webpack-dev-server is not running for development mode.
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.
Yes, added some notes in the README. Anything missing?
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.
The doc for the method should be updated (or removed? with a reference to the readme.)
* master: Update React pointers to include react on rails
@@ -32,6 +32,9 @@ def javascript_pack_tag(*names, **options) | |||
# in config/webpack/shared.js. By default, this list is auto-generated to match everything in | |||
# app/javascript/packs/*.js. In production mode, the digested reference is automatically looked up. | |||
# | |||
# Note: If the development server is running and hot module replacement is active, this will return nothing. | |||
# In that setup you need to configure your styles to be inlined in your JavaScript for hot reloading. | |||
# |
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.
@justin808 You mean this doc?
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.
Maybe adjust this line to indicate more than "development mode":
# # In development mode:
# <%= stylesheet_pack_tag 'calendar', 'data-turbolinks-track': 'reload' %> # =>
# <link rel="stylesheet" media="screen" href="/packs/calendar.css" data-turbolinks-track="reload" />
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.
Done 👍
Lowercase
* master: Move to a helper and fix tests (#648) Spacing
Extracted from #601