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

RFC: remove React Transform from examples #1455

Closed
wants to merge 1 commit into from
Closed

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Feb 28, 2016

Inspired by @thejameskyle’s rant on Twitter (no offense taken 😄 ), I propose to remove babel-preset-react-hmre from examples for the following reasons:

  1. Both plugin transform and proxy wrapping are rather invasive and have edge cases
  2. It’s experimental tech and beginners shouldn’t have to deal with its warts
  3. It doesn’t currently reload functional components or container functions like mapStateToProps
  4. In Redux we rarely care about preserving component local state when hot reloading! Preserving store state is usually enough.

This is a proof of concept of “vanilla” hot reloading (for now, only for a single example).

It has the following traits.

Pros

  1. It is absolutely non-invasive and doesn’t change semantics of your code
  2. It hot reloads any components, whether classes or functions
  3. It preserves the store state on hot reload
  4. It still handles errors on mount and hot reloading “fixes” them
  5. It works for functions like mapStateToProps

Cons

  1. It does not preserve the local state of components
  2. It does not preserve the DOM so the components are remounted

What do you think? Should we proceed with this and do this for every example?

@17cupsofcoffee
Copy link

I think this is a good idea - having the transform plugin in the examples sorta gives the impression that it's something that you need to have in order to get hot reloading working with Redux. In fact, I was kinda under that impression myself until reading this!

@gaearon
Copy link
Contributor Author

gaearon commented Feb 28, 2016

Yeah a lot of people tend to get that impression.

@ghost
Copy link

ghost commented Feb 28, 2016

Given all of this talk about "JavaScript fatigue", the less dependencies you need, the better.

That said, I definitely want to see react-transform advertised heavily as a DX improvement/plugin for redux - once you get comfortable using vanilla redux.

</Provider>,
rootEl
)
}

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.

@ericclemmons
Copy link

👍 from me. I agree that this doesn't belong in these examples, since it's not directly-related to Redux.

@ghost
Copy link

ghost commented Feb 28, 2016

Sounds like a good move to me

@dlmanning
Copy link

This is what we've drifted towards as our project has gotten larger. Component HMR is just too finicky to maintain, and all the state we care about preserving is in the store

@frontsideair
Copy link

I like this, it demonstrates there's nothing magic about hmre.

@Phoenixmatrix
Copy link

This is 100x better: handles all the common scenarios for 1/10th of the complexity. I'm in love with it.

document.getElementById('root')
)
let render = () => {
const App = require('./containers/App').default

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.

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.

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.

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

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.

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

@tanepiper
Copy link

👍 from me, the hmre plugin caused me a lot of headaches a couple of weeks back (mostly documented here: https://gist.github.com/tanepiper/ea41aca1dda6473af738). It was a weird issue I never did resolve in the end, except to switch back to vanilla react-transform-hmr which worked out the box as expected.

@dayyeung
Copy link

@gaearon I just tried your modified example, RedBox shows correctly when an error is thrown, but after removing the error, the following occurs: Uncaught TypeError: Cannot read property '_currentElement' of null, and hot reloading no longer works. Is it a bug or is it not supported?

edit: It is actually the opposite, simply throwing an error on hot reload will break it.

@gaearon
Copy link
Contributor Author

gaearon commented Feb 29, 2016

@dayletter

Good catch, this indeed happens with stable React. I was testing with the master version of React. I checked 0.15.0-alpha.1 and it doesn’t seem to have this problem. I would imagine that this was fixed with the initial pass on adding error boundaries in https://github.com/facebook/react/pull/5602/files#diff-c807d0f5ebbc12c44d9479dea693bef6R379.

@imranismail
Copy link

Tried with alpha react, the same error happens

@gaearon
Copy link
Contributor Author

gaearon commented Feb 29, 2016

Weird. Are you sure it was 0.15 alpha, you installed it in example’s own folder and that you restarted the server?

@0x80
Copy link

0x80 commented Mar 9, 2016

@sunyang713 It only relies on standard webpack hmr. You can use both the hot middleware and the standard dev server with hmr enabled.

@Dema
Copy link

Dema commented Mar 12, 2016

Guys,so what is the most current way to use react hot reload with redux? Do you have any boilerplace project? https://github.com/gaearon/react-transform-boilerplate seems to be kind of deprecated.

@gaearon
Copy link
Contributor Author

gaearon commented Mar 12, 2016

What you see in this PR is what I recommend for Redux apps after React 15 comes out. I hope to work on a more complete solution that would replace RT later.

@@ -15,7 +15,6 @@ module.exports = {
plugins: [
new webpack.optimize.OccurenceOrderPlugin(),
new webpack.HotModuleReplacementPlugin(),
new webpack.NoErrorsPlugin()

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?

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.

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

@gaearon
Copy link
Contributor Author

gaearon commented Mar 13, 2016

@sunyang713

Dev server vs express middleware is completely irrelevant to hot reloading. The only reason I prefer middleware in new examples is because it's easier to show how to combine it with server rendering than when you have two separate servers. You can use WebpackDevServer absolutely fine with either this setup or React Transform. This choice is completely unrelated.

@silvenon
Copy link

I really benefit from React Transform even though I'm using Redux, because lots of times I'm using component state for things that are awkward to keep in the Redux store, like toggling a modal. A modal is an example where React Transform shines, because sometimes you need a few clicks to open a modal again, so without it you would have to make a few clicks every time you save a file.

@zhangmq
Copy link

zhangmq commented Apr 15, 2016

I want to build isomorphic app with react-router async routes, failed with function component too. I found this hmr solution, but it's hard to implement Root for async routes. I write code below, it works fine, but does not look good. need some advise for better solutions.

hot wrapper

const hot = (r, a) => {
  if (module.hot) {
    let Inner = r();

    const wrapped = class Wrapped extends Component {
      constructor(props) {
        super(props);

        a(() => {
          Inner = r();
          this.forceUpdate();
        });
      }

      render() {
        return <Inner {...this.props} />;
      }
    };

    return wrapped;
  }

  return r();
};

routes

{
    path: '/',
    component: hot(
      () => require('./containers/Application/Application'),
      (update) => module.hot.accept('./containers/Application/Application', update)
    ),
    indexRoute: {
      getComponent(nextState, cb) {
        require.ensure([], require => {
          cb(null, hot(
            () => require('./containers/Home/Home'),
            (update) => module.hot.accept('./containers/Home/Home', update)
          ));
        });
      }
    },
    childRoutes: [
      {
        path: 'about',
        getComponent(nextState, cb) {
          require.ensure([], require => {
            cb(null, hot(
              () => require('./containers/About/About'),
              (update) => module.hot.accept('./containers/About/About', update)
            ));
          });
        },
        onEnter: requireLogin
      }
    ]
  }

@crysislinux
Copy link

A note:

I think it's better to also wrap the reducer with try catch block to avoid broken HMR.

if (module.hot) {
  // Enable Webpack hot module replacement for reducers
  module.hot.accept('../reducers', () => {
    try {
      const nextRootReducer = require('../reducers').default;
      store.replaceReducer(nextRootReducer);
    } catch (error) {
      /* eslint-disable no-console */
      console.error('error: ', error);
      /* eslint-enable no-console */
    }
  });
}

markerikson added a commit to markerikson/react-redux-cesium-testing-demo that referenced this pull request Apr 18, 2016
Per reduxjs/redux#1455, use of the "plain"
Webpack HMR API for hot-loading can be simpler and easier to manage.
@gaearon
Copy link
Contributor Author

gaearon commented Apr 18, 2016

Hey everyone!

While I’m still probably going to keep vanilla HMR in Redux examples for simplicity, I just released an alpha of React Hot Loader 3 which handles functional components, needs very little configuration (especially with regards to server rendering or production builds), works great with higher order components (so you can hot reload mapStateToProps) and contains other improvements.

It’s not 100% ready yet but I encourage you to check it out!

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

@timdorr
Copy link
Member

timdorr commented Jun 26, 2016

@gaearon Can we close this out then?

@timdorr
Copy link
Member

timdorr commented Aug 10, 2016

Superseded by #1883

@Sawtaytoes
Copy link

I saw a comment here about react-router being supported only with synchronous routes:
https://github.com/gaearon/react-hot-boilerplate/blob/next/src/App.js

Is it possible it works with async routes now? I moved my code into a separate root.jsx file to match the examples, and it works so long as I swap my <Router /> component for a <div />. This works with async routes in 1.3.0.

Sadly, on-save with the code as-is, it refreshes the page :/.

client.jsx (entry)

import React from 'react'
import { render } from 'react-dom'
import { match } from 'react-router'
import { AppContainer } from 'react-hot-loader'

// Root Component
import Root from 'root'

// Polyfills
import 'react-fastclick'
import 'utilities/polyfills'

// Store and Routes
import { history } from 'utilities/store'
import routes from './routes'

// Router
match({ history, routes }, (error, redirectLocation, renderProps) => {
    if (redirectLocation) {
        window.location = redirectLocation.pathname
    }

    render(
        <AppContainer>
            <Root renderProps={renderProps} />
        </AppContainer>
    , document.getElementById('root'))

    if (module.hot) {
        module.hot.accept('./root', () => {
            const RootContainer = require('./root').default
            render(
                <AppContainer>
                    <RootContainer renderProps={renderProps} />
                </AppContainer>
            , document.getElementById('root'))
        })
    }
})

root.jsx

import React, { Component } from 'react'
import { Provider } from 'react-redux'
import { browserHistory, Router } from 'react-router'

// Store and Routes
import { store } from 'utilities/store'

export default class Root extends Component {
    render() { return (
        <Provider store={store}>
            <Router history={browserHistory} {...this.props.renderProps} />
        </Provider>
    )}
}

Repo w/ Update Branch:
https://github.com/Sawtaytoes/Ghadyani-Framework-Webpack-React-Redux/tree/feature/update-react-hot-loader

Update Commit:
Sawtaytoes/Ghadyani-Framework-old@c675c42

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

Successfully merging this pull request may close these issues.

None yet