-
Notifications
You must be signed in to change notification settings - Fork 751
add auto hot reload support #1065
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
Conversation
This reverts commit ad27aee.
| var renderWithReactDOM = require("./src/renderComponent/withReactDOM") | ||
| var renderWithHotReload = require("./src/renderComponent/withHotReload") |
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.
As mentioned in the add render component extension point PR, the folder structure and import pattern is taking reference from getConstructor for consistency. Please let me know if there is a better way
| // Enables hot reload for component rendering. | ||
| // | ||
| // See the HMR section in README to ensure required steps are completed. | ||
| useHotReload: function(requireContext) { | ||
| this.renderComponent = renderWithHotReload(requireContext) | ||
| }, |
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.
Let me know if this comment could be more helpful, i.e. should the steps to setup be inlined here?
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.
In addition, there is an opportunity to "stack" instead of "replace". See the example and discussion at #1064. The proposed approach here mirrors getConstructor and useContext. Let me know if the "stack" option would be a better option
| // Render React component with hot reload. | ||
| // | ||
| // See the HMR section in README to ensure required steps are completed. |
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.
Same for this one. Let me know if this comment could be more helpful, i.e. should the steps to setup be inlined here?
| 1. install [react-hot-loader](https://github.com/gaearon/react-hot-loader) and [@hot-loader/react-dom](https://github.com/hot-loader/react-dom) | ||
| 2. add the following to your webpack config in dev: | ||
| ```js | ||
| { | ||
| module: { | ||
| rules: [ | ||
| { | ||
| test: /\.(jsx|tsx)?$/, | ||
| use: ["react-hot-loader/webpack"], | ||
| }, | ||
| ], | ||
| }, | ||
| resolve: { | ||
| alias: { | ||
| "react-dom": "@hot-loader/react-dom", | ||
| }, | ||
| }, | ||
| } | ||
| ``` | ||
| 3. in your entry file, usually where you call `ReactRailsUJS.useContext` already, call `useHotReload`: | ||
| ```js | ||
| var ReactRailsUJS = require("react_ujs") | ||
| var myCustomContext = require.context("custom_components", true) | ||
| ReactRailsUJS.useHotReload(myCustomContext) | ||
| ``` | ||
| 4. optionally, for CSS to hot reload, update the following for dev in `webpacker.yml`: | ||
| ```yml | ||
| development: | ||
| <<: *default | ||
| extract_css: false | ||
| ``` |
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.
Let me know if the steps in readme is concise and simple to follow. Happy to pull into a separate file if requested
|
I'll need to test it doesn't negatively affect any existing integration but this looks promising so thank you for contributing! Travis succeeded on all builds except Webpack 4 so I'll need to take a look at that. |
|
@EdmondChuiHW whats the status of this PR? would love to have access to this. |
@danvernon looks good to me 😜 Ask the author to see if there's anything I can do to help merge this |
|
@BookOfGreg anything stopping this being merged? |
|
We need a solution that uses: https://github.com/pmmmwh/react-refresh-webpack-plugin/ Per https://github.com/gaearon/react-hot-loader, react-hot-loader is no longer recommended. The new plugin is much less intrusive to your source code. It does not require changing the source code, only the development build configuration to use webpack and babel plugins. Shakapacker supports HMR with React: |
requires #1064 to be merged
Summary
Automatically hot reload all components rendered with
react_ujswithout source file changes. Existing support requires manually wrapping all components withhot(App). This PR removes the need.Other Information
Inline comments have been added to discuss options considered and their tradeoffs.
related: facebook/react#16604
Also uncertain on how to fix the Travis CI build error
Steps to enable (also updated in README)
ReactRailsUJS.useContextalready, calluseHotReload:webpacker.yml: