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

Upgrade to webpack v2 #1291

Merged
merged 15 commits into from
Feb 11, 2017
Merged

Upgrade to webpack v2 #1291

merged 15 commits into from
Feb 11, 2017

Conversation

Timer
Copy link
Contributor

@Timer Timer commented Dec 18, 2016

},
// JSON is not enabled by default in Webpack but both Node and Browserify
// allow it implicitly so we also enable it.
{
test: /\.json$/,
loader: 'json'
loader: 'json-loader'
Copy link
Contributor

Choose a reason for hiding this comment

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

json-loader is no longer required in Webpack 2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

You should take it out of the package.json file as well.

@@ -81,12 +81,12 @@ module.exports = {
// We use `fallback` instead of `root` because we want `node_modules` to "win"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the comment change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably -- we don't need people to be confused.

@@ -134,7 +134,7 @@ module.exports = {
/\.json$/,
/\.svg$/
],
loader: 'url',
loader: 'url-loader',
query: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Although webpack 2 allows query, it has chosen to use options in its docs which IMO is better, would you consider changing?

// splitting or minification in interest of speed. These warnings become
// cumbersome.
performance: {
hints: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is turned off completely during development and by default set to warning in the production config, it might come as a surprise to users to then have to tweak their app for code-splitting, although extracting css should bring the file size down. @TheLarkInn gives a reason to leave it enabled in dev builds. We could tweak the maxAssetSize and maxEntrypointSize options to reasonable file sizes instead. What do you think?

Copy link
Contributor Author

@Timer Timer Dec 19, 2016

Choose a reason for hiding this comment

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

I don't think it's very valuable in its certain form -- I think we need to go a step further and show our own warnings more in line with what @TheLarkInn was saying.

Currently, we don't display build size warnings -- and bundle size is going to vary wildly between end-users. I don't think there is a good "default" we can set which will cover all cases (and we don't want to expose configuration unless it's a feature we want to support for the foreseeable future of cra; even if we switch bundlers).

Knowing this, I think that monitoring build size between rebuilds would be beneficial to let the user know their bundle has grown -- maybe doing some caching so we can display warnings between restarts/sessions.

Since the feature isn't there now, I think we can safely disable it completely until we implement said feature or wait for more detailed [json] performance hints from webpack.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note, performance hints are now off by default in rc4:
https://github.com/webpack/webpack/releases/tag/v2.2.0-rc.4

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be set to warning in production now?

Choose a reason for hiding this comment

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

What kind of additional details are you looking for? At the least I'd recommend having it as "warning" in Production as well.

Copy link
Contributor

@bondz bondz left a comment

Choose a reason for hiding this comment

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

Remove unused json-loader from the package.json file.

{
test: /\.json$/,
loader: 'json'
loader: 'style-loader!css-loader?importLoaders=1!postcss-loader'
Copy link
Contributor

Choose a reason for hiding this comment

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

To apply multiple loaders, webpack 2 uses the use option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Out of curiosity, is this old syntax still supported?

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't seem to work. webpack treats the whole string as a single loader throwing a Module not found: Error: Can't resolve ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! (That syntax is so confusing.)

Copy link
Contributor

Choose a reason for hiding this comment

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

use

use: [
  'style-loader', 
  {
    loader: 'css-loader',
    options: {
      importLoaders: 1
    }
  },
  'postcss-loader'
]

@existentialism
Copy link
Contributor

existentialism commented Dec 19, 2016

Probably need to bump a few deps for peerDep warnings:

And wait for some pending version bumps:

@@ -213,6 +187,30 @@ module.exports = {
inject: true,
template: paths.appHtml,
}),
new webpack.LoaderOptionsPlugin({
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we shouldn't have this section and instead pass options to loaders. (If those loaders support the new API.)

loader: ExtractTextPlugin.extract('style', 'css?importLoaders=1!postcss')
loader: ExtractTextPlugin.extract({
fallbackLoader: 'style-loader',
loader: 'css-loader?importLoaders=1!postcss-loader'
Copy link
Contributor

Choose a reason for hiding this comment

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

This could also be written as

loader: [
            {
              loader: 'css-loader',
              options: {
                importLoaders: 1
              }
            },
            'postcss-loader'
          ]

@bondz
Copy link
Contributor

bondz commented Dec 20, 2016

Some other dependencies are outdated, should those deps be updated as well? In a separate PR, I could send one.

@Timer
Copy link
Contributor Author

Timer commented Dec 20, 2016

Some other dependencies are outdated, should those deps be updated as well? In a separate PR, I could send one.

I've always wondered the same and if we should track these dependencies through their new versions.
I feel like the general consensus is if it works, leave it as it is. If we update dependencies we might introduce breakages due to bugs in package updates.
To update dependencies safely, we need extensive unit testing beyond the current smoke test -- this is on the horizon.

@wtgtybhertgeghgtwtg
Copy link
Contributor

Would that mean enabling Greenkeeper after #1187 lands?

@Timer
Copy link
Contributor Author

Timer commented Dec 21, 2016

We'll have to open a discussion issue about it -- I can think of plenty of reasons to argue for and against it.
However, with an appropriate test suite, I don't believe it could hurt.


Anyway, this PR is currently blocked on waiting for a few dependencies to update. Hopefully webpack 2.x is fully released by this time. 😄

@kpuputti
Copy link

kpuputti commented Jan 4, 2017

The linked html-webpack-plugin PR is now merged and released, so that dependency can be checked. \o/

@Timer
Copy link
Contributor Author

Timer commented Jan 5, 2017

Neat, thanks for the update @kpuputti!

@existentialism
Copy link
Contributor

existentialism commented Jan 13, 2017

Can bump to extract-text-plugin@v2.0.0-beta.5 now! 🙌

(Should continue tracking a few blockers to release tho)

@kirkaustin
Copy link

Webpack 2.2.0 has been released.

@Timer
Copy link
Contributor Author

Timer commented Jan 19, 2017

Thanks @existentialism -- I've added the biggest two blockers for us to the OP.


Thanks @kirkaustin, I've updated this PR accordingly. 👍

@gaearon gaearon added this to the 0.10.0 milestone Jan 23, 2017
@gaearon
Copy link
Contributor

gaearon commented Jan 23, 2017

Tagging this for 0.10 together with other major features.

"webpack": "1.14.0",
"webpack-dev-server": "1.16.2",
"webpack": "2.2.0",
"webpack-dev-server": "2.2.0-rc.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

webpack-dev-server 2.2.0 was released a few days ago :).

@sashakru
Copy link

This two issues are closed:

webpack-contrib/extract-text-webpack-plugin#265
webpack-contrib/extract-text-webpack-plugin#281

loader: [
{
loader: 'css-loader',
query: {
Copy link

Choose a reason for hiding this comment

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

query -> options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That change is pending on webpack-contrib/extract-text-webpack-plugin#281, which I believe has gone through.

@Timer
Copy link
Contributor Author

Timer commented Feb 11, 2017

CI passing 🎉

@Timer
Copy link
Contributor Author

Timer commented Feb 11, 2017

🚀 let's do it!

@Timer Timer merged commit 1228883 into facebook:master Feb 11, 2017
@Timer Timer deleted the webpack-v2 branch February 11, 2017 19:11
@gaearon
Copy link
Contributor

gaearon commented Feb 11, 2017

@Timer Timer changed the title Upgrade webpack Upgrade to webpack v2 Feb 11, 2017
bondz pushed a commit to bondz/create-react-app that referenced this pull request Feb 12, 2017
* Upgrade webpack

* Address more webpack v2 ...

* Update html-webpack-plugin

* Remove LoaderOptionsPlugin from dev config

* ExtractTextPlugin still uses webpack 1 syntax
... and doesn't support complex options (yet)

* Grammar nit

* Update extract text webpack plugin

* - Remove webpack.LoaderOptionsPlugin
- Update deps

* Lerna hoists packages

* Update extract-text-webpack-plugin

* Update webpack-dev-server

* Remove imports for the tests

* stop removing babelrc
@sublimeye
Copy link

sublimeye commented Feb 20, 2017

@Timer Is this change should be already available when installing a new app? I'm still getting webpack@1.14 with a fresh installation.

Created a bug just in case #1599

@gaearon
Copy link
Contributor

gaearon commented Feb 20, 2017

Just because something was merged, doesn't mean it was released 😉 .

@Timer
Copy link
Contributor Author

Timer commented Feb 20, 2017

No @sublimeye, it is only available in master and must be installed manually.

edit: please be aware that using master is strictly for testing purposes and not considered stable, see @gaearon's comment below.

@gaearon
Copy link
Contributor

gaearon commented Feb 20, 2017

Just to be clear, I wouldn't recommend installing it manually to anyone. We're still working out the issues, and master is never considered fully stable.

You can see the milestone for each issue and PR:

screen shot 2017-02-20 at 18 03 38

Compare it to the latest release to know if something is released or not.

danielfigueiredo pushed a commit to danielfigueiredo/create-react-app that referenced this pull request Feb 22, 2017
* Upgrade webpack

* Address more webpack v2 ...

* Update html-webpack-plugin

* Remove LoaderOptionsPlugin from dev config

* ExtractTextPlugin still uses webpack 1 syntax
... and doesn't support complex options (yet)

* Grammar nit

* Update extract text webpack plugin

* - Remove webpack.LoaderOptionsPlugin
- Update deps

* Lerna hoists packages

* Update extract-text-webpack-plugin

* Update webpack-dev-server

* Remove imports for the tests

* stop removing babelrc
@axelson axelson mentioned this pull request Feb 23, 2017
"eslint-plugin-flowtype": "2.21.0",
"eslint-plugin-import": "2.0.1",
"eslint-plugin-jsx-a11y": "2.2.3",
"eslint-plugin-react": "6.4.1",
"extract-text-webpack-plugin": "1.0.1",
"extract-text-webpack-plugin": "2.0.0-rc.3",
Copy link
Contributor

@SimenB SimenB Mar 2, 2017

Choose a reason for hiding this comment

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

@gaearon
Copy link
Contributor

gaearon commented Mar 2, 2017

@shai32

When everything tagged for 0.10 is ready. We don’t delay releases because we want to—we delay them because there are outstanding bugs (in CRA or underlying tools), or because there are features we still want to get in. Maybe you want to help?

kst404 pushed a commit to kst404/e8e-react-scripts that referenced this pull request Mar 2, 2017
* Upgrade webpack

* Address more webpack v2 ...

* Update html-webpack-plugin

* Remove LoaderOptionsPlugin from dev config

* ExtractTextPlugin still uses webpack 1 syntax
... and doesn't support complex options (yet)

* Grammar nit

* Update extract text webpack plugin

* - Remove webpack.LoaderOptionsPlugin
- Update deps

* Lerna hoists packages

* Update extract-text-webpack-plugin

* Update webpack-dev-server

* Remove imports for the tests

* stop removing babelrc
@shai32
Copy link

shai32 commented Mar 3, 2017

Sorry for asking for release date, will not do it again.
Thanks for anyone who contribute to this project.

@gaearon
Copy link
Contributor

gaearon commented Mar 3, 2017

No worries, I’m sorry if my message sounded combative 😞 .

I just meant that we don’t really have any specific day in mind—it’ll take some time to fix all the issues, and help is welcome.

@iddan
Copy link
Contributor

iddan commented Mar 12, 2017

For some reason even though I upgraded the package it still installs me webpack 1

@gaearon
Copy link
Contributor

gaearon commented Mar 12, 2017

Have you had a chance to read the comments above asking the very same question?
😉

@viksit
Copy link

viksit commented Mar 21, 2017

@gaearon @Timer how do you recommend manually installing CRA from github? I've tried npm install -g git+https://github.com/facebookincubator/create-react-app.git but that results in a

npm ERR! No name provided in package.json

npm ERR! A complete log of this run can be found in: ...

What am I doing wrong?

@gaearon
Copy link
Contributor

gaearon commented Mar 21, 2017

We don't support installing from GitHub (and that won't help you anyway because the CLI is not where the code lives).

Please don't rush to migrate to Webpack 2 because it's cool. We'll release support for it when it's fully ready. Give it some time please.

If you absolutely insist on trying it, there's an unstable, possibly buggy, nightly canary build of react-scripts that includes Webpack 2. I will lock this thread and won't tell you how to find it because if it's posted here, then every beginner will attempt to use it, and their projects will break in unexpected ways. But if you know how to find unstable npm package versions, then feel free to give it a try and report any issues 😉 .

@facebook facebook locked and limited conversation to collaborators Mar 21, 2017
@gaearon
Copy link
Contributor

gaearon commented May 16, 2017

Please help beta test the new version that includes this change!
#2172

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

Successfully merging this pull request may close these issues.