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

Consider using Webpack 2 #183

Closed
gaearon opened this issue Jul 25, 2016 · 31 comments
Closed

Consider using Webpack 2 #183

gaearon opened this issue Jul 25, 2016 · 31 comments

Comments

@gaearon
Copy link
Contributor

gaearon commented Jul 25, 2016

It’s under active development, and from what I’ve heard, most bugs have shaken out by now. I’d like to see a PR porting this project to Webpack 2 so that we may compare them, and either to switch to it, or file issues against it 😄 . I’d expect such PR to use Webpack 2’s native support for ES Modules.

If you plan to work on this, please don’t forget to comment here so we don’t have duplicate PRs.

@mxstbr
Copy link
Contributor

mxstbr commented Jul 25, 2016

I'd love to do this, we ported react-boilerplate over a few months and have had 0 issues. If nobody else gets to it first I'll do it!

@7rulnik
Copy link

7rulnik commented Jul 25, 2016

I did it for react-boilerplate, so, it's time for create-react-app!

@ChristopherBiscardi
Copy link

ChristopherBiscardi commented Jul 25, 2016

hah, I was just working on this (before finding this issue). @7rulnik there's a branch here you can cop from if you find it useful: https://github.com/ChristopherBiscardi/create-react-app/tree/webpack-2

@mxstbr
Copy link
Contributor

mxstbr commented Jul 25, 2016

Sorry, I was in the middle of it already when I read this! See #189, feel free to continue the work against the webpack-v2 branch.

@taion
Copy link

taion commented Jul 26, 2016

I don't think this is as good of an idea as it might seem at first:

  • webpack 2 may be stable enough for general use, but it's still marked as a beta, and thus not bound by semver guarantees, so there's no promise of stability from versioning
  • For users who eject, it's going to be very difficult to find documentation on using webpack 2 – specifically, almost all docs out there are going to still be for webpack 1, and it's confusing as heck
  • Just turning on webpack 2 doesn't give you the benefits of tree-shaking the way you think
    • By default, webpack 2 doesn't look at jsnext:main, so packages exposing ES module builds for rollup don't get their ES module builds used (in fact I'm not sure if any ES module builds from packages will get used in a default webpack 2 config)
      • Even if you add it, it turns out that a ton of package maintainers just stick their untranspiled source in jsnext:main, which is actually just broken/wrong/won't work
    • For most packages, tree-shaking yields limited benefit because of the limitations of static analysis; e.g. in React Router, it will not prevent pulling in the random singleton histories because it can't be statically determined that their instantiation doesn't have side effects... likewise for React-Bootstrap because of the HoC patterns used there (though once I realized the ES module build for React Router was pointless, I didn't go through with adding one to R-B anyway)
      • (Apparently eslint-plugin-lodash fixes this though? – but does so by rewriting imports? cc @jdalton)

So essentially, webpack 1 is tried and tested and relatively easy to find documentation on. webpack 2 seems pretty stable, but it's harder to learn right now, and the path forward to tree-shaking is fraught and yields not terribly many benefits.

@jdalton
Copy link

jdalton commented Jul 26, 2016

(Apparently eslint-plugin-lodash fixes this though? – but does so by rewriting imports? cc @jdalton)

Do you mean babel-plugin-lodash or lodash-webpack-plugin?

@taion
Copy link

taion commented Jul 26, 2016

babel-plugin-lodash, sorry.

(This package inspired me to muck around with my build tooling, and I got confused.)

@jdalton
Copy link

jdalton commented Jul 26, 2016

Ah cool. babel-plugin-lodash works for non-lodash packages too. You just specify an "id" in its plugin options to be that of your package or as an array of package ids you want it to work for.

"plugins": [
  ["lodash", { "id":  ["async", "lodash-bound", "ramda"] }]
]

@taion
Copy link

taion commented Jul 26, 2016

That's super cool.

To be concrete, even if you point at the React Router ES6 build in the current v3 pre-release in webpack 2, if you just do:

import { Router } from 'react-router/es6'; // Or use jsnext:main.

You will end up pulling in a bunch of code that you don't use, including stuff for e.g. the hash history, all the link and route helper components, &c.

In practice, tree-shaking is not enough to let you change the way you write imports, unless you use babel-plugin-lodash or something similar.

For a number of popular libraries, users should not rely on tree-shaking as built into the bundlers, and must find some other way to import just what they need, if they want a properly size-optimized bundle.

This means that, from my perspective, tree-shaking support also always has to come with a warning that goes along the lines of "don't rely on this – you still need to do deep imports in most cases".

There still is a benefit to using bundlers with native ES module support, but that benefit is restricted to getting rid of the Babel transpilation cruft.

@mxstbr
Copy link
Contributor

mxstbr commented Jul 26, 2016

Even if you add it, it turns out that a ton of package maintainers just stick their untranspiled source in jsnext:main, which is actually just broken/wrong/won't work

FWIW we have that enabled for react-boilerplate, we have a lot of dependencies and only one of the broke but was fixed within hours. I'm fine with not adding that for now since I agree that the ecosystem might not be stable enough.

almost all docs out there are going to still be for webpack 1, and it's confusing as heck

That's true, but the differences are tiny AND webpack warns you when you have something wrongly configured in the 1.x way. I do agree this might become a problem, though I know there's a lot of effort happening to make the webpack docs much better.

(Also, resolve.packageMains is now resolve.mainFields: https://github.com/webpack/docs/wiki/Configuration#resolvepackagemains)

@taion
Copy link

taion commented Jul 26, 2016

I immediately hit breakage when I cut over my webpack config to webpack 2 earlier today.

I think that's there's a pretty significant cost to impose potential breakage on inexperienced users with packages that should work, especially when the reasons for that breakage are going to be very hard for a user who doesn't know what they're doing in track down (i.e. syntax errors in node_modules).

@taion
Copy link

taion commented Jul 26, 2016

Okay, what?

The default supported ES module entry point hook in webpack 2 is actually 'module', per https://github.com/dherman/defense-of-dot-js/blob/master/proposal.md, from discussion in webpack/webpack#1979.

The Rollup resolver plugin, on the other hand, doesn't know about 'module' and only knows about 'jsnext:main'.

What the heck are library developers supposed to do? Clutter up package.json files with both? @gaearon are you planning on populating the module key in the Redux package definition?

@mxstbr
Copy link
Contributor

mxstbr commented Jul 26, 2016

The default supported ES module entry point hook in webpack 2 is actually 'module', per https://github.com/dherman/defense-of-dot-js/blob/master/proposal.md, from discussion in webpack/webpack#1979.

You can actually set that with webpack:

// webpack.config.js
{
  // …
  resolve: {
    mainFields: ['module', 'jsnext:main'] // Used to be resolve.packageMains
  }
}

@taion
Copy link

taion commented Jul 26, 2016

I know – that's how I managed to hit breakage there earlier.

I think using module instead of jsnext:main here would actually be a better idea. It's one less config thing in case users eject, and hopefully we can do a better job of educating library maintainers that they definitely should not point module at the untranspiled source, as was done with jsnext:main.

My issue with jsnext:main isn't just that it might cause errors – it's that the errors would be utterly inscrutable to someone not otherwise familiar with what's going on.

@gaearon
Copy link
Contributor Author

gaearon commented Jul 26, 2016

To be clear, the goal of migration is not to enable tree shaking. Right now it doesn't concern me much.

The goal is to get to a place where we can quickly contribute and make fixes to Webpack without worrying about porting them to different branches.

The reason I'm not too worried about semver is because we lock the version anyway, and we don't expose config to the user. But maybe I'm not seeing the risks?

@kentcdodds
Copy link
Contributor

I think upgrading to Webpack 2 is great. Even getting some tree shaking is better than none. And as @gaearon said it's not really a primary goal anyway, just nice when it happens. Also, the API is nicer.

As for things being confusing and having a lack of documentation for Webpack 2 when people eject, I recommend we use webpack-validator to help with that. Another thing that could be handy is webpack-config-utils which makes the config compose better (and read easier) without abstracting the Webpack API.

@taion
Copy link

taion commented Jul 26, 2016

@kentcdodds @gaearon

My main concern is that this puts users in an incredibly difficult situation post-eject.

We all like to talk about how webpack docs aren't great. Actually, they're not that bad. I picked up webpack 1 more or less from scratch not terribly long ago by looking at some combination of the docs site, Stack Overflow questions, and the plethora of existing examples.

None of these exist with webpack 2. I'd like to think I know what I'm doing now (more or less), and I had to cobble together a webpack config by going to a combination of parenthetical mentions in GitHub Wikis, an old Gist, and checking the webpack source code and open issues.

I think anybody who's not already familiar with this ecosystem, when faced with a webpack 2 config after ejecting, will be, uhh, "totally screwed" is how I'd describe it – they'd not only have to figure out how to use webpack 2 from limited and spread out documentation... they'd also have to know to disregard irrelevant documentation on webpack 1.

Post-eject DX obviously shouldn't be a primary concern here, but I think that switching to webpack 2 so thoroughly compromises DX that I don't think it's a good thing to do.

@kentcdodds
Copy link
Contributor

You make good points @taion. I think that my opinion should be taken with a grain of salt because I do a lot of training and teaching about webpack and have spent hours (days/weeks?) worth of time learning and developing with it. For me, the changes from webpack 1 to webpack 2 were fairly minimal. But perhaps it would be better to wait until Webpack 2 is officially released and the docs situation is sorted out.

@gaearon
Copy link
Contributor Author

gaearon commented Jul 26, 2016

This is a great point. Let’s hold this off until Webpack 2 is at least out of beta then.
This shouldn’t take long, I hope.

@gaearon
Copy link
Contributor Author

gaearon commented Sep 30, 2016

We’ll also need to do this: #613

@tbillington
Copy link

Just commenting in here that webpack 2 rc has released.

@gaearon
Copy link
Contributor Author

gaearon commented Dec 15, 2016

Let me know if you'd like to revive #189.

@Timer Timer mentioned this issue Dec 19, 2016
10 tasks
@ianschmitz
Copy link
Contributor

Looks like webpack 2 officially hit final status!

@treyhuffine
Copy link

@ianschmitz @gaearon

Looks like a great time to revive this. I've been working through the upgrade myself. Once it's ready, I'll submit a PR if no one else has.

@ianschmitz
Copy link
Contributor

Looks like they have a PR going on over at #1291

@Timer
Copy link
Contributor

Timer commented Feb 11, 2017

Merged as of today. #1291 🎉

@Timer Timer closed this as completed Feb 11, 2017
@stereobooster
Copy link
Contributor

stereobooster commented May 1, 2017

When this will be published on npm?

UPD: oh, I see it is in milestone 1.0.0.

For future googlers: to use it right now yarn add react-scripts@canary. To check available version npm view react-scripts dist-tags

As always found answer in @gaearon tweet

@johann_sonntag @timer150 You can check the "milestone" field on any PR to learn which release it is slated for

— Dan Abramov (@dan_abramov) February 26, 2017

@gaearon
Copy link
Contributor Author

gaearon commented May 1, 2017

When this will be published on npm?

When 0.10 is ready. (I’ll be looking at it this week.)

For future googlers: to use it right now yarn add react-scripts@canary.

No, please don’t do this unless you really know what you’re doing. There is a reason canary releases aren’t released as stable: they are buggy and not supported. It’s fine to try them but you should expect things to break.

@onpaws
Copy link

onpaws commented May 4, 2017

Lovely to hear this is getting attention! Thanks for your efforts.

@gaearon
Copy link
Contributor Author

gaearon commented May 16, 2017

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

@xyzdata
Copy link

xyzdata commented Jun 20, 2017

keep up with time!

@lock lock bot locked and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests