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

Webpack 2 #861

Merged
merged 90 commits into from Nov 6, 2015
Merged

Webpack 2 #861

merged 90 commits into from Nov 6, 2015

Conversation

@sokra
Copy link
Member

sokra commented Mar 6, 2015

No description provided.

sokra and others added 27 commits Jan 12, 2015
Conflicts:
	lib/Parser.js
	package.json
pass --env to config function
… into webpack-2

Conflicts:
	lib/Compilation.js
@jhnns
Copy link
Member

jhnns commented Mar 6, 2015

Woot woot woot!

Hottest changes (taken from the commit log) are:

  • switched to acorn for it's better es6 support
  • ES6 parser, ES6 modules
  • disable full dynamic require by default (require(expr))

and

  • test it too 😉

Did you experience any performance benefits/penalties from switching to acorn? What is actually breaking with 2.0.0?

@jhnns
Copy link
Member

jhnns commented Mar 6, 2015

Btw: I tested this branch in a fairly big project and it worked without changing anything. 👍

@sokra
Copy link
Member Author

sokra commented Mar 6, 2015

What is actually breaking with 2.0.0?

Only the uncool stuff... If you are only using cool stuff, it propably still works...
But there are more changes coming...

@klimashkin
Copy link

klimashkin commented Jan 8, 2016

@sokra I have a quick question about tree-shaking
Assume, that we have folder /pages with dozen of modules and index.js module in it with re-exporting list of all other modules like

export module1 from './module1';
export module2 from './module2';

And some upper-level script makes import {module1} from './pages';. Will module2 be eliminated from build and from page/index.js re-export list with tree-shaking?

@sokra
Copy link
Member Author

sokra commented Jan 8, 2016

@klimashkin no. Only the exports of module2 will be omitted.

Removing module2 totally would be an unsafe operation, because module2 could contain side-effects that wouldn't be executed.

I plan to offer is kind of unsafe optimization as opt-in plugin, but there is nothing there yet.


In my opinion the ES6 spec failed here as it doesn't allow this optimization... (Would be difficult to apply in a dynamic system, but possible in a static system like webpack)

@klimashkin
Copy link

klimashkin commented Jan 8, 2016

You mean only re-export will be omitted, or both - export and re-export but not module itself?

@sokra
Copy link
Member Author

sokra commented Jan 8, 2016

export and re-export but not module itself?

yes

@sokra
Copy link
Member Author

sokra commented Jan 8, 2016

Omitting exports of a module allows uglifys dead code elimination to remove side-effect free code.

@JamesHenry
Copy link

JamesHenry commented Jan 8, 2016

@sokra is there somewhere specific where we should look out for more info on Webpack 2?

I am happy to help start something if not. It would obviously be great for everyone involved if there was somewhere other than this merged PR where knowledge could be shared and you were not responsible for answering ad hoc questions :)

I was already able to start using webpack 2 on my app without any functional problems which is great, but I had to remove it again as the npm errors from things like webpack-dev-server became too much whenever installing new dependencies. I can see @dtothefp had the same experience.

Would it possible to get webpack2 versions of other "webpack products" like the dev server set up on npm?

PS. Thanks again for all your amazing work on webpack - for me, it is one of the most impressive frontend projects around

@dtothefp
Copy link

dtothefp commented Jan 8, 2016

@JamesHenry I agree would be great to have a beginning knowledge share i.e. New stuff and deprecated plugins/syntax. Also as I mentioned earlier a beta version on npm would be useful because for large projects installing dependency directly from github repo is a no go.

As for your dev server issues switching to webpack middleware and hot middleware may solve your problems, seems to be a more configurable alternative

https://github.com/webpack/webpack-dev-middleware
https://www.npmjs.com/package/webpack-hot-middleware

@sokra
Copy link
Member Author

sokra commented Jan 8, 2016

It would obviously be great for everyone involved if there was somewhere other than this merged PR where knowledge could be shared and you were not responsible for answering ad hoc questions :)

I'll write a larger article about webpack 2 once it's finished.

PS: Here is more info, for the curious one: https://docs.google.com/document/d/1tRc0MzvRdGK7EbG2LRW8vSyoxKhR_EvRUz3AQRyFZso/edit

@kentcdodds
Copy link
Member

kentcdodds commented Jan 8, 2016

@sokra, I'm interested in why this was decided:

webpack differs from the ES6 modules spec: imports are not hoisted

Seems like a bad call to me, but I'm sure I'm out of context.

@kentcdodds
Copy link
Member

kentcdodds commented Jan 8, 2016

Also, for:

  • add “es6:main” field as default package main
    • Name?

I may have missed earlier discussion on this, but for the name I thought I'd just bring up the fact that we'll need to revisit this again every year for each new version of the spec. I imagine this has been considered, but I thought I'd just bring it up just in case. May not be a bad thing.

@phaistonian
Copy link

phaistonian commented Jan 9, 2016

@sokra Wondering: will the "tree shaking" support result in bundle sizes comparable to the ones Rollup claims to get?

Because, the way I see it, that's their main selling point.

@sokra
Copy link
Member Author

sokra commented Jan 9, 2016

webpack differs from the ES6 modules spec: imports are not hoisted

In this example:

imports "a";
require("b");
imports "c";

The Spec says execution order is: a, c, b.

Currently webpack differs from the spec by using the execution order: a, b, c. I hope this makes incremental transition to ES6 easier.


ill the "tree shaking" support result in bundle sizes comparable to the ones Rollup claims to get?

Sadly not. webpack need to support CJS and AMD too. These styles are pretty dynamic, which requires wrapping in functions to control the execution time. An idea is to add ES6 modules as output target to webpack (still difficult) and run rollbar on the result.

@phaistonian
Copy link

phaistonian commented Jan 9, 2016

@sokra Running rollbar on the result sounds the most "logical" way to go, though mashing up 2 bundlers would feel a bit ..dirty.

Is there an estimation of % slower bundling in production will be since tree shaking will be involved?

Oh, one little thing I noticed (could be because of glitch) is that the optimize.OccurenceOrderPlugin fails (not found) in 2.0.

Will re-check on this one later though.

ps: yay for auto applying NODE_ENV=production on -p :)

@danieljuhl
Copy link

danieljuhl commented Jan 9, 2016

@phaistonian Have you seen issue #53 in webpack docs? webpack/docs#53
OccurenceOrderPlugin -> OccurrenceOrderPlugin

@dtothefp
Copy link

dtothefp commented Jan 9, 2016

@phaistonian if you want to read more fine grained details you should have a look here rollup/rollup#219 (comment). @sokra actually pitches in to this conversation on roll up. Seems to me roll up is a good solution if you are only worried about making es6 JS bundles with no extras and will also be a nice tool if you are using http2 and doing only light bundling of commonly grouped modules. For the rest of us who use http1 and also, more importantly, use webpack for many invaluable tools such as scss compilation/local css/public path replacement, image/font handling via file & url loader, patching of non commonjs/AMD non compliant modules, and isomorphic compilation I think webpack will remain an invaluable tool.

If you are really concerned about boilerplate bloating your code you can look into CommonsChunksPlugin and generate a vendor bundle that you can cache heavily. Not sure how much this helps with the substitution of exports/imports from es6 to commonjs syntax in each file but it does chunk out some webpack runtime as far as I can tell and eliminates potentially bundling twice a library that is imported into multiple bundle entry points

@sokra
Copy link
Member Author

sokra commented Jan 9, 2016

is that the optimize.OccurenceOrderPlugin fails (not found) in 2.0.

No longer needed, is default.

@phaistonian
Copy link

phaistonian commented Jan 10, 2016

@dtothefp Thank you for your pointers and input.

I do use CommonsChunksPlugin and, for what it's worth, I believe Webpack is the most exciting technology we have seen lately (this and React).

It's not about comparing bundlers, it's about being excited as to what could be done (if any) in order to get the best of both worlds in the world you need/love the most (Webpack).

@sokra: Speaking of defaults, perhaps HotModuleReplacementPlugin should also come as default (one day) in dev environment.

@graue
Copy link

graue commented Jan 26, 2016

Do I need any special configuration to enable System.import? Tried it and it just compiles directly to System['import'], then fails at runtime with ReferenceError: System is not defined.

@sokra
Copy link
Member Author

sokra commented Jan 31, 2016

@dtothefp
Copy link

dtothefp commented Jan 31, 2016

@sokra awesome....thanks for the gist!!!

@BenoitZugmeyer
Copy link

BenoitZugmeyer commented Jan 31, 2016

@sokra it looks awesome!

  • I would rewrite the System.import example in Code Splitting with ES6. Since the .catch does not return anything, the .then will be called with undefined as first argument if the module fails to load.
  • s/comsuming/consuming
@sokra
Copy link
Member Author

sokra commented Feb 1, 2016

@BenoitZugmeyer thanks, updated it.

@kentcdodds
Copy link
Member

kentcdodds commented Feb 1, 2016

I've forked it and fixed various spelling and grammatical errors (see changes here, in some cases I'm being nit-picky just because I expect this document will be used for documentation in the future). Also changed CommonJs to CommonJS as that's the most common casing (from what I can tell).

I had some items of feedback/questions if it's not too late:

  • Seems odd that the --env flag behaves differently if there's only one vs having multiple. I would prefer that it always results in an object. I feel like keeping it consistent would help reduce confusion.
  • I'm unsure of the use/need of aliasFields when you can already provide an array of mainFields. The document didn't seem to explain it well enough for me at least.
  • Rename moduleExtensions to moduleSuffix. In my mind extension maps to a file extension (like .js or .css). A suffix seems to be more appropriate for what this is actually for (this change would also require that enforceModuleExtension is changed to enforceModuleSuffix).
  • the configuration file resp. thecontextoption I wasn't sure what this phrase is supposed to say. What is resp. short for?

Looks great! I'm really excited to try this out :D

@karlhorky
Copy link

karlhorky commented Feb 1, 2016

Maybe resp. is short for 'respective of'?

@sokra
Copy link
Member Author

sokra commented Feb 2, 2016

Thanks for the corrections, merged your fork.

Seems odd that the --env flag behaves differently if there's only one vs having multiple. I would prefer that it always results in an object. I feel like keeping it consistent would help reduce confusion.

I just pass along the value I get from CLI parsing (yargs). Objects are possible because the lib support them (https://www.npmjs.com/package/yargs#dot-notation).
--env abc => "abc"
--env => true
--env 123 => 123
--env.hello => {hello: true}
--env.hello world => {hello: "world"}

I'm unsure of the use/need of aliasFields when you can already provide an array of mainFields. The document didn't seem to explain it well enough for me at least.

I added an additional sentense.

"browser": {
  "fs": false,
  "./index.js": "./browser.js"
}
@kentcdodds
Copy link
Member

kentcdodds commented Feb 2, 2016

Thanks for answering my questions @sokra! I think I misunderstood the --env flag. I'm good with what you've described 👍

- // The content of these field is an object where requests to a key are mapped to the corresponding value
+ // The content of this field is an object where requests to a key are mapped to the corresponding value

I hope you're ok with the nit-picking. Let me know if this isn't the time or place to be nit-picky and feel free to ignore my suggestions altogether :-)

Also, what do you think of my other suggestion?

Rename moduleExtensions to moduleSuffix. In my mind extension maps to a file extension (like .js or .css). A suffix seems to be more appropriate for what this is actually for (this change would also require that enforceModuleExtension is changed to enforceModuleSuffix).

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

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.