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.

Sign up for GitHub

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

Jump to bottom

RFC: remove React Transform from examples #1455

Closed
wants to merge 1 commit into from
Closed

RFC: remove React Transform from examples #1455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 1 addition & 6 deletions examples/async/.babelrc
View file
Open in desktop
@@ -1,8 +1,3 @@
{
"presets": ["es2015", "react"],
"env": {
"development": {
"presets": ["react-hmre"]
}
}
"presets": ["es2015", "react"]
}
44 changes: 36 additions & 8 deletions examples/async/index.js
View file
Open in desktop
@@ -1,15 +1,43 @@
import 'babel-polyfill'
import React from 'react'
import { render } from 'react-dom'
import ReactDOM from 'react-dom'
Copy link

Choose a reason for hiding this comment

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

Why use ReactDOM.render() instead of plain render()?

Copy link
Contributor

Choose a reason for hiding this comment

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

render is being assigned on line 10...

import { Provider } from 'react-redux'
import App from './containers/App'
import configureStore from './store/configureStore'

const store = configureStore()
const rootEl = document.getElementById('root')

render(
<Provider store={store}>
<App />
</Provider>,
document.getElementById('root')
)
let render = () => {
const App = require('./containers/App').default
Copy link

Choose a reason for hiding this comment

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

What's wrong with leaving this as an ES2015 import? Is there a technical reason why this must be changed to an inline require?

Copy link
Contributor

Choose a reason for hiding this comment

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

the loader spec is still a work in progress but webpack v2 understands the draft proposal, which allows inline (dynamic) imports using system loader, ie:

// webpack v2
let render = () => {
  System.import('./containers/App').then(App => {
    const AppContainer = App.default
    ReactDOM.render(
      <Provider store={store}>
        <AppContainer />
      </Provider>,
      rootEl
    )
  })
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason it’s an inline require is because we want to import the latest version after hot update.
Babel transpiles import to require() so we have no opportunity to require() again.

In Webpack 2, if we disable Babel modules transform, we will be able use a normal import because Webpack will keep it a live binding and update the imported value. Webpack 2 isn’t quite ready yet though so we don’t rush the migration.

Copy link

Choose a reason for hiding this comment

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

Also, import statements have to be top-level, not nested.

Copy link

Choose a reason for hiding this comment

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

Thanks @gaearon makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tomduncalf @mgcrea

Please see the discussion: here's how to make it work with React Router.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've been following the discussion about vanilla HMR and decided to use that approach when upgrading to webpack in my universal-dev-toolkit.

I'm using react-router with a separate routes-file and a RouterContainer that serves as the root-component which is reloaded in the app.js-file using the approach in this PR. It all seems to work well for me so far. When I add a new view to the router, everything refreshes accordingly. Perhaps this could serve as a starting point for someone wishing to implement it themselves?

If you want to add Redux into the toolkit, you can wrap the <Provider store={store}> around the RouterContainer like @0x80 did (see previous comment).

Copy link

Choose a reason for hiding this comment

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

Thanks @gaearon for the pointer and @stoikerty for the information. I'm still struggling to get this working - when updating a component, I either get the component doesn't know how to update itself message if I say module.hot.accept('containers/Root'...), or Warning: [react-router] You cannot change <Router routes>; it will be ignored if I say module.hot.accept('routes'...) (which is the component containing the routes).

I suspect that what is happening is that there is no clear "include path" from my component up to the root component due to the way my app is structured, hence the doesn't know how to update message. There is a clear path from the updated component to the Routes component, but whatever reason routes doesn't want to be reloaded.

I need to take a look at my setup in more detail and work out why this might be happening... not a big deal for now anyway, I'm sticking with class components and react-transform for this project :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@tomduncalf If you provide a more comprehensive example where it doesn't work, say in a repo or a gist, I'm willing to have a look for you. It sounds like the component you're trying to update might be outside of the tree that is on the hot-reload path.

Copy link

Choose a reason for hiding this comment

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

Thanks @stoikerty, that is my suspicion too - my structure is a little more complex than the examples, because I have split the application into separate modules (which are in essence individual single page applications, with their own entry point) but with common code for some parts.

I will put together a minimal reproducible example when I have a moment, and it would be great to get your feedback if I am unable to get it working! Thanks

ReactDOM.render(
<Provider store={store}>
<App />
</Provider>,
rootEl
)
}
Copy link

Choose a reason for hiding this comment

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

Why not put the try-catch here and call render() below so even if you reload and get a redbox it will still update with new changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good call. My goal was to make sure RedBox doesn't get into the production build but I can put some more conditionals to ensure that.


if (module.hot) {
// Support hot reloading of components
// and display an overlay for runtime errors
const renderApp = render
const renderError = (error) => {
const RedBox = require('redbox-react')
ReactDOM.render(
<RedBox error={error} />,
rootEl
)
}
render = () => {
try {
renderApp()
} catch (error) {
renderError(error)
}
}
module.hot.accept('./containers/App', () => {
setTimeout(render)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need setTimeout there? I have never seen it in other hot reload examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the original motivation was to avoid errors thrown in render blowing up HMR updates. Now we handle those errors in try/catch so maybe it’s not necessary anymore. I’ll look into it again after React 15 comes out and maybe remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the information! I am working on my own hotloader for something, so I wonder how it is done in other implementations.

Copy link

Choose a reason for hiding this comment

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

hey, how are you able to use the old render function without changing the contents of the render? doesn't render use the un-updated App component?

Following the HMR code for reducers in your other examples, wouldn't the callback of module.hot.accept() need to look like this:

// render takes in a component to render
let render = (App) => ReactDOM.render(App, rootEl)

 /* . . . */

module.hot.accept('./containers/App', () => {
  const NextApp = require('./containers/App').default
  render(NextApp)
})

Copy link
Contributor

Choose a reason for hiding this comment

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

Line 11 const App = require('./containers/App').default always gets the updated component. Inline require calls do that by their very nature of being a function instead of a top-level import. The fact that the module isn't passed into render doesn't matter in this case because as soon as the render-function is run, the variable definition inside render in turn runs the require-function that triggers a re-require of the module.

Even though the code inside render is the same, the functions that are called within render can still produce a different result. You might be confusing class-constructors and regular functions.

})
}

render()
2 changes: 1 addition & 1 deletion examples/async/package.json
View file
Open in desktop
Expand Up @@ -40,10 +40,10 @@
"babel-loader": "^6.2.0",
"babel-preset-es2015": "^6.3.13",
"babel-preset-react": "^6.3.13",
"babel-preset-react-hmre": "^1.0.1",
"expect": "^1.6.0",
"express": "^4.13.3",
"node-libs-browser": "^0.5.2",
"redbox-react": "^1.2.2",
"webpack": "^1.9.11",
"webpack-dev-middleware": "^1.2.0",
"webpack-hot-middleware": "^2.2.0"
Expand Down
1 change: 0 additions & 1 deletion examples/async/webpack.config.js
View file
Open in desktop
Expand Up @@ -15,7 +15,6 @@ module.exports = {
plugins: [
new webpack.optimize.OccurenceOrderPlugin(),
new webpack.HotModuleReplacementPlugin(),
new webpack.NoErrorsPlugin()
],
Copy link

Choose a reason for hiding this comment

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

@gaearon Why did you remove NoErrorsPlugin?

Copy link

Choose a reason for hiding this comment

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

He's displaying them with react-redbox instead (see line 24). If he continued using the NoErrorsPlugin, Webpack would suppress the errors, and Redbox would just never be rendered anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is not the reason. Redbox only catches runtime errors, it doesn’t help with syntax errors.
NoErrorsPlugin has upsides and downsides. I’m likely to add it back. Please see gaearon/react-transform-boilerplate#122 for a list of its upsides and downsides.

Copy link

Choose a reason for hiding this comment

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

That makes more sense, sorry for the incorrect explanation

module: {
loaders: [
Expand Down