-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Optimize the library for webpack users #1517
Conversation
(I’d also be happy to copy & expand the webpack section into http://caolan.github.io/async/ if this gets merged.) |
This repo is really used to build 2 modules: Unfortunately, the |
Lol. I should’ve looked for that repo :D I’ve just tried testing this with 1. I’d still add the 2. As far as I understood, READMEs are shared between the packages. Would it make sense to link to both A-la “This project exists in two versions. If you use Node.js, install (Ideally, I’d add two different READMEs with slightly different code examples tailored for each version. If you’re open for that, I could tune the build process to do that) |
Both 1 and 2 would be great. We do a bad job of advertising the existence of |
1e40422
to
f2443eb
Compare
Just force-pushed the updates. Looking forward to your review :–) |
README.es.md
Outdated
|
||
```javascript | ||
// for use with callbacks... | ||
import { forEachFor } from "async-es"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this should be import { forEachOf } from "async-es";
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, thanks! Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from one small typo, looks good.
(Pushed fixes a few days ago :–) |
Merged, thanks! |
Hey there! I optimized the library for users who build their browser apps with webpack. webpack is super-popular in front-end development, so that should be a large percent of browser users.
In my tests with
map
/parallel
methods, the library size dropped from 28 to 12 minified kB (−57%). Here’s the repo if you want to reproduce: iamakulov/async-optimization-reproThis is a part of my performance consulting. I help open-source projects (for free) and companies (for money) to optimize the performance of their products: iamakulov.com/perf-consulting
The problem
Right now, when you import
async
into the app, you import all the methods it includes. So even if you only want to use.map
, your app receives all 78async
methods:Usually, this is solved by importing only the necessary methods:
However, in case with
async
, this doesn’t help because theasync
’s entry point,dist/async.js
, is a bundle with all the library’s code.Right now, there’s no easy solution to this problem except doing
import map from 'async/map'
, but that’s not very obvious.The solution
I added a few flags that hint webpack how to optimize the library:
Added
"module": "lib/index.js"
intopackage.json
When the
module
field is present, webpack uses it instead ofmain
to determine the entry point into the lib. Withmodule
, webpack would consume ES modules and drop unused code with tree shaking.Added
"sideEffects": false
intopackage.json
sideEffects: false
tells webpack that library modules don’t have any side effects (e.g., don’t modify any globals). Webpack uses this info to remove the module if it’s reexported but not used. This improves the effect of tree-shaking.Removed
lib
from.npmignore
Webpack users would need this directory, so we’ll need to publish it to npm.
This has a drawback – the module would get larger and take longer to install – but, as a benefit, browser users would spend less time downloading it. Browser downloads happen more often than npm installs, so I believe this is for good.
Documented the browser usage in readme. Docs are located at http://caolan.github.io/async/, but I’ve thought the performance considerations are important enough to place the snippet in the readme (many users won’t get into docs far enough to read it there). If you don’t want this change, I’d be happy to move it elsewhere.
(Sorry for ads in the beginning, I do freelancing for living)