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

Build improvements #13

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Build improvements #13

wants to merge 16 commits into from

Conversation

srgpqt
Copy link

@srgpqt srgpqt commented Nov 8, 2015

Webpack has builtin functionality to do pretty much everything the Gruntfile does ; it makes no sense to use two different tools that do the same thing.

These commits rectify most of that situation.

@benoitvallon
Copy link
Owner

Thank you, that's a very interesting (and big) PR full of improvements.

I had a look at your modifications and here are the few details I found.

  • The livereload mecanism is broken in the NW.js App (as you use the same index.html, you removed the script to livereload.js)
  • It would be great if you could launch the website app behind the http://localhost:8000/webpack-dev-server/index.html url, it would have 2 advantages, first it would keep the "App hot update" banner, and second just after the first launch while webpack is building the app for the first time it would indicate it on the web page instead of having a empty web page.

Then I have some open questions:

  • I am not sure that having a common index.html file (for web and nw.js) is the best option because the templating language makes the file hard to read, some content shouldn't be on the NW.js side (the IE condition <!--[if lt IE 8]> for example as we know the engine included with NW) and I think some meta tags are irrelevant too inside a NW.js App.
  • Other question related to the previous, how I you do here to have different titles between Apps, would I need to customize it in the webpack.config.js file? <title>{%=o.htmlWebpackPlugin.options.title || 'Webpack App'%}</title>

I really like the way you reorganized the webpack config files, it makes the whole thing cleaner and easier. What you did with the package.web.json is a very good idea too.
And I am glad that very soon I won't have the npm warnings about those peer deps ;)

Impressive work.

])
});

module.exports.module.loaders[0].loader = 'react-hot!babel-loader';
Copy link
Owner

Choose a reason for hiding this comment

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

I think something like this is more readable and easy to understand because it reflects your idea of aggregating config (you add one loader to the previous ones at the beginning of the list)

loaders: baseConfig.module.loaders.unshift({
  test: /\.js$/,
  exclude: /node_modules/,
  loader: 'react-hot'
})

Copy link
Author

Choose a reason for hiding this comment

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

That wouldn't work because unshift returns the new array length, not the actual array. You'd have to use .concat for that.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh yes sorry the version that I tested worked because I saved the result in the loaders property instead of module.loaders and the baseConfig object was still updated by the method.

As you mentionned a .concat version of it would be fine like:

module: {
  loaders: Array.prototype.concat([{
      test: /\.js$/,
      exclude: /node_modules/,
      loader: 'react-hot'
    }],
    baseConfig.module.loaders)
}

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

Successfully merging this pull request may close these issues.

None yet

2 participants