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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hot loader and universal rendering, a lot of refactoring #31

Merged
merged 4 commits into from Jul 31, 2017

Conversation

Projects
None yet
4 participants
@Stanko
Member

Stanko commented Jun 21, 2017

TODO for this PR

  • Universal rendering
  • De/hydrate the state
  • Switch to react-hot-loader v3
  • Inline SVG sprite (external doesn't work with IE10 and IE11 馃槩)
  • Import SVGs as components
  • Enable CSS source maps
  • Update readme
  • Update to webpack 3.0

Things that are related but that are NOT going to be a part of this PR

  • React helmet for meta data
  • Async data fetching on universal server
  • Server hmr (is this possible?) [1]

[1] Currently it watches and builds server and client bundles. nodemon task (express server) is restarted whenever server bundled is changed.

Universal stuff is completely optional.

This is rather large PR and I would appreciate greatly if you guys can review. Thank you!

How to test

Use these tasks to test:

  • npm start (or npm run client:dev if you want fancy dashboard) starts and watches only client application on webpack dev server (hmr enabled)
  • npm run server:dev starts and watches only express server to test universal rendering (no hmr)
  • server-and-client:dev starts and watches both client and server (no hmr)

Where to go from here?

Unfortunately boilerplate became opinionated as it does a lot of things. If you guys agree, maybe we should just go with what we think are the best options, instead of making a generator? TBH I'm not sure where to start with generators, and boilerplate is hard enough to maintain as it is. Thoughts?

@oliverdore-work @nemanjawork @fcaneto @edwintoh-work @rafaelrinaldi @sarahatwork @chralden @marwan-at-work

@Stanko Stanko referenced this pull request Jun 21, 2017

Closed

Universal rendering #22

@nemanjawork

This comment has been minimized.

Show comment
Hide comment
@nemanjawork

nemanjawork Jun 21, 2017

Member

LGTM

Member

nemanjawork commented Jun 21, 2017

LGTM

if (module.hot) {
module.hot.accept('./views/Client/', () => {
const NewClient = require('./views/Client/index').default; // eslint-disable-line global-require

This comment has been minimized.

@edwintoh-work

edwintoh-work Jun 21, 2017

if we're using webpack 2, I think we don't have to do this.

We can just do:


  module.hot.accept('./views/Client/index', () => {
    render(Client);
  });
@edwintoh-work

edwintoh-work Jun 21, 2017

if we're using webpack 2, I think we don't have to do this.

We can just do:


  module.hot.accept('./views/Client/index', () => {
    render(Client);
  });

This comment has been minimized.

@Stanko

Stanko Jun 22, 2017

Member

I tried that first, but it doesn't work, as Client component is wrapped in redux's Provider. This was only way to get both app and redux hot reload.

@Stanko

Stanko Jun 22, 2017

Member

I tried that first, but it doesn't work, as Client component is wrapped in redux's Provider. This was only way to get both app and redux hot reload.

const render = Component => {
ReactDOM.render(
<AppContainer>
<Provider store={ store }>

This comment has been minimized.

@edwintoh-work

edwintoh-work Jun 21, 2017

I haven't caught up to the RR4 game yet. But is there a concept of history and do we need it?

@edwintoh-work

edwintoh-work Jun 21, 2017

I haven't caught up to the RR4 game yet. But is there a concept of history and do we need it?

This comment has been minimized.

@Stanko

Stanko Jun 22, 2017

Member

Good catch, there is, I'll add it.

@Stanko

Stanko Jun 22, 2017

Member

Good catch, there is, I'll add it.

@edwintoh-work

@Stanko good stuff! I left a couple of small comments. LMK what you think!

"react",
"stage-0"
"stage-0",
"react"
],
"env": {

This comment has been minimized.

@rafaelrinaldi

rafaelrinaldi Jun 21, 2017

Member

Might be worth adding optimizations like react-optimize for the production step here.

@rafaelrinaldi

rafaelrinaldi Jun 21, 2017

Member

Might be worth adding optimizations like react-optimize for the production step here.

This comment has been minimized.

@Stanko

Stanko Jun 22, 2017

Member

Good idea, funny how it outputs larger app at the end 馃槃

@Stanko

Stanko Jun 22, 2017

Member

Good idea, funny how it outputs larger app at the end 馃槃

render(Client);
if (module.hot) {
module.hot.accept('./views/Client/', () => {

This comment has been minimized.

@rafaelrinaldi

rafaelrinaldi Jun 21, 2017

Member

I would strongly recommend switching from HMR to the default Webpack dev server live reload. Pretty much the same benefits, without the hackery and maintenance burden.

@rafaelrinaldi

rafaelrinaldi Jun 21, 2017

Member

I would strongly recommend switching from HMR to the default Webpack dev server live reload. Pretty much the same benefits, without the hackery and maintenance burden.

This comment has been minimized.

@Stanko

Stanko Jun 22, 2017

Member

Don't know much abou it, adn I'll look into it, but react hot loader is cool as it can update reducers as well.

@Stanko

Stanko Jun 22, 2017

Member

Don't know much abou it, adn I'll look into it, but react hot loader is cool as it can update reducers as well.

historyApiFallback: true,
port: 3000,
compress: IS_PRODUCTION,
inline: !IS_PRODUCTION,

This comment has been minimized.

@rafaelrinaldi

rafaelrinaldi Jun 21, 2017

Member

I think you don't have to use dynamic values here since this configuration will only be loaded up in dev mode anyway.
I would also enable the overlay configuration here that has been added on Webpack 2 and can be very helpful for debugging.

@rafaelrinaldi

rafaelrinaldi Jun 21, 2017

Member

I think you don't have to use dynamic values here since this configuration will only be loaded up in dev mode anyway.
I would also enable the overlay configuration here that has been added on Webpack 2 and can be very helpful for debugging.

This comment has been minimized.

@Stanko

Stanko Jun 22, 2017

Member

Added overlay, wasn't aware that exists.

Dynamic values are there to enable "preview" mode, where dev server serves built application.

@Stanko

Stanko Jun 22, 2017

Member

Added overlay, wasn't aware that exists.

Dynamic values are there to enable "preview" mode, where dev server serves built application.

This comment has been minimized.

@rafaelrinaldi

rafaelrinaldi Jun 22, 2017

Member

Gotcha, makes sense.

@rafaelrinaldi

rafaelrinaldi Jun 22, 2017

Member

Gotcha, makes sense.

"hook-add": "prepush install",
"hook-remove": "prepush remove"
"hook-remove": "prepush remove",
"test": "echo \"Error: no test specified\" && exit 1"
},
"devDependencies": {

This comment has been minimized.

@rafaelrinaldi

rafaelrinaldi Jun 21, 2017

Member

Might be a good idea to have a deps task that will make it easier to keep track of dependencies. You can use something like npm-check or updtr.

@rafaelrinaldi

rafaelrinaldi Jun 21, 2017

Member

Might be a good idea to have a deps task that will make it easier to keep track of dependencies. You can use something like npm-check or updtr.

This comment has been minimized.

@Stanko

Stanko Jun 22, 2017

Member

Cool, I will look into that.

@Stanko

Stanko Jun 22, 2017

Member

Cool, I will look into that.

},
}),
new webpack.NamedModulesPlugin(),
// Builds index.html from template
new HtmlWebpackPlugin({

This comment has been minimized.

@rafaelrinaldi

rafaelrinaldi Jun 21, 2017

Member

Can also enable minification to the HTML file itself:

minify: {
 collapseWhitespace: true,
 minifyCSS: true,
 minifyJS: true,
 removeComments: true,
 useShortDoctype: true,
},
@rafaelrinaldi

rafaelrinaldi Jun 21, 2017

Member

Can also enable minification to the HTML file itself:

minify: {
 collapseWhitespace: true,
 minifyCSS: true,
 minifyJS: true,
 removeComments: true,
 useShortDoctype: true,
},
@rafaelrinaldi

Added a bunch of minor comments but overall looks good. Great work!

To your point I would say better defaults > scaffolding/configuration.

@Stanko

This comment has been minimized.

Show comment
Hide comment
@Stanko

Stanko Jun 21, 2017

Member

Thanks everyone, I'll go tomorrow through all of the feedback 馃憤

Member

Stanko commented Jun 21, 2017

Thanks everyone, I'll go tomorrow through all of the feedback 馃憤

@Stanko Stanko merged commit 06e3869 into master Jul 31, 2017

@Stanko Stanko deleted the hot-loader-and-universal branch Jul 31, 2017

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