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

Modularization #996

Merged
merged 39 commits into from Feb 24, 2016
Merged

Modularization #996

merged 39 commits into from Feb 24, 2016

Conversation

@megawac
Copy link
Collaborator

megawac commented Jan 3, 2016

A work in progress to bring modular support and lodash implementations of certain methods.

Previous discussion be found here #984.

Resolves #984

See https://github.com/caolan/async/tree/modularization/build for the generated bundles


TODOS

  • script releases:
    • generate modular builds/files
    • copy ES modules to build/es
    • add git commit
    • npm test
    • smoke test build files
    • cd build/
    • publish npm release
    • revert modular build changes, returning async to normal folder structure
    • git tag, git push
  • possibly? create npm release for each module
  • #984 (comment)
  • use baseOrderBy in the implementation of internal/flatten?
  • move documentation to jsdoc format (bonus?) save for later
  • test package with npm pack before first 2.0 publish
Kikobeats and others added 5 commits Dec 15, 2015
Add .DS_Store

Extract util methods

Fix createTester callback

Fix notid exports

Refactor settimediate and nexttick

Fix dep path

Create bundle using browserify

Rename main file

Rename main file

Rename main file

Use browserify standalone mode

Modular interface for main methods 📦

Deleted unnecessary test

Add script to generate modules package.json

Delete noconflict module

Improve how to generat browser bundle

Update util modules references

Add a way to generate module scaffold

Fix version

Remove unnecessary dependencies

Require the dependency

Add missing methods

Add dependencies for each module

Bumped 0.4.0

Add useful scripts

Add .npmignore

Updated

Add npmignore files

Refactor

Fix little issues

Fix library name

Fix isarray module

Update script

Fix typo

Fix template links

Update deps

Revert "Fix template links"

This reverts commit 112a382.

Bump 0.5.1

Fix mapseries reference

Bump 0.5.2
@megawac

This comment has been minimized.

Copy link
Collaborator Author

megawac commented Jan 4, 2016

I have managed to remove most of the string parsing logic included by lodash -- the only string parsing code remaining is toNumber included by baseRange. @jdalton may consider moving the toNumber logic to range and having baseRange assume it is provided numeric start, end and step?

Also @jdalton it'd be nice if there was a stable ref I could link for a lodash-es submodule -- it's pretty frustrating to update the submodule ref every couple days

@jdalton

This comment has been minimized.

Copy link
Contributor

jdalton commented Jan 4, 2016

may consider moving the toNumber logic to range and having baseRange assume it is provided numeric start, end, and step?

Good idea!

it'd be nice if there was a stable ref I could link for a lodash-es submodule -- it's pretty frustrating to update the submodule ref every couple days

Lodash v4 will be released on the 12th.

@megawac

This comment has been minimized.

Copy link
Collaborator Author

megawac commented Jan 4, 2016

Cool, so there will be a stable es-4.0.0 ref in v4?
On Jan 4, 2016 9:32 AM, "John-David Dalton" notifications@github.com
wrote:

may consider moving the toNumber logic to range and having baseRange
assume it is provided numeric start, end, and step?

Good idea!

it'd be nice if there was a stable ref I could link for a lodash-es
submodule -- it's pretty frustrating to update the submodule ref every
couple days

Lodash v4 will be released on the 12th.


Reply to this email directly or view it on GitHub
#996 (comment).

@jdalton

This comment has been minimized.

Copy link
Contributor

jdalton commented Jan 4, 2016

Cool, so there will be a stable es-4.0.0 ref in v4?

Yep, just as there's a stable 3.10.1 ref.

@megawac

This comment has been minimized.

Copy link
Collaborator Author

megawac commented on build/async-cjs.js in 85f5be9 Jan 4, 2016

@Rich-Harris is there an option to disable this default check?

This comment has been minimized.

Copy link

Rich-Harris replied Jan 4, 2016

@megawac not presently. We should probably replace these with something similar to Babel's interopRequire

This comment has been minimized.

Copy link

Rich-Harris replied Jan 4, 2016

have opened an issue for it rollup/rollup#419

@aearly

This comment has been minimized.

Copy link
Collaborator

aearly commented Jan 5, 2016

What does the testing situation look like? Do we just test the bundle? It might be handy to test the es6 modules directly somehow as well, just to verify that Async works in both forms.

@megawac

This comment has been minimized.

Copy link
Collaborator Author

megawac commented Jan 5, 2016

@jdalton

This comment has been minimized.

Copy link
Contributor

jdalton commented Jan 7, 2016

Updated the es branch with toNumber removed from baseRange.
The gzipped minified size is now 6.17 kB so a 1.9 kB difference which I think is fine.

@megawac

This comment has been minimized.

Copy link
Collaborator Author

megawac commented Jan 7, 2016

Nice! 👍 that is totally reasonable in my opinion, thoughts @aearly?

@aearly aearly added this to the 2.0 milestone Jan 7, 2016
@jdalton

This comment has been minimized.

Copy link
Contributor

jdalton commented Jan 7, 2016

Found another cheap win which will bring the size down to 5.73 kB; the diff to 1.4 kB
5.87 kB; the diff of 1.6 kB :P

@aearly

This comment has been minimized.

Copy link
Collaborator

aearly commented Jan 7, 2016

Really excellent 👍 . Lodash is totally worth 1.4kb 😛 .

The only danger I see is relying on an internal lodash function, but those are pretty stable, right?

@jdalton

This comment has been minimized.

Copy link
Contributor

jdalton commented Jan 7, 2016

The only danger I see is relying on an internal lodash function, but those are pretty stable, right?

It depends. When relying on internals I usually avoid the ^ range and go with the ~.
I can review the deps and report back if I see anything of concern. Offhand I think the internals used here are stable.

@megawac

This comment has been minimized.

Copy link
Collaborator

megawac commented on support/build/plugin-lodash-import-rename.js in 1f79497 Jan 8, 2016

can be even simpler and import lodash-es/

This comment has been minimized.

Copy link
Collaborator

megawac replied Jan 9, 2016

never mind, the reason I was using submodules was so rollup would properly bundle lodash

This comment has been minimized.

Copy link
Collaborator

aearly replied Jan 9, 2016

Yeah, I tried getting rollup to automatically bundle lodash-es from npm, but couldn't get it working. I need to familiarize myself with all the plugins and options more.

@megawac

This comment has been minimized.

Copy link
Collaborator

megawac commented on 1f79497 Jan 8, 2016

Nice thanks

This comment has been minimized.

Copy link
Collaborator

aearly replied Jan 8, 2016

This is really odd, I don't know how @Kikobeats showed up as the commit author here. I'm the one working on this. 😄

This comment has been minimized.

Copy link
Collaborator

aearly replied Jan 8, 2016

Oh wait, I accidentally cherry-picked a merge commit (the .editorconfig). That caused some weirdness.

This comment has been minimized.

Copy link
Collaborator

megawac replied Jan 8, 2016

Haha, I was wondering where this commit came from.

@megawac

This comment has been minimized.

Copy link
Collaborator

megawac commented on 1f79497 Jan 8, 2016

Nice thanks

This comment has been minimized.

Copy link
Collaborator

aearly replied Jan 8, 2016

This is really odd, I don't know how @Kikobeats showed up as the commit author here. I'm the one working on this. 😄

This comment has been minimized.

Copy link
Collaborator

aearly replied Jan 8, 2016

Oh wait, I accidentally cherry-picked a merge commit (the .editorconfig). That caused some weirdness.

This comment has been minimized.

Copy link
Collaborator

megawac replied Jan 8, 2016

Haha, I was wondering where this commit came from.

@jdalton

This comment has been minimized.

Copy link
Contributor

jdalton commented Jan 8, 2016

Just checked and the base methods used are totally stable. You should have no problems with them.
I also simplified the deps of toNumber.

megawac and others added 2 commits Jan 10, 2016
@megawac

This comment has been minimized.

Copy link
Collaborator Author

megawac commented Feb 18, 2016

The last question in my mind is what do we want our main file to be? The
built file (to avoid imports) or the CJS file (current)

On Thu, Feb 18, 2016 at 2:06 PM, Alex Early notifications@github.com
wrote:

I guess now that the code review is complete we can ignore them.


Reply to this email directly or view it on GitHub
#996 (comment).

@aearly

This comment has been minimized.

Copy link
Collaborator

aearly commented Feb 18, 2016

The CJS index seems more "correct", but it would only be better in node-land. If you're browserify-ing the UMD build will be smaller, and it will still wok in node. I say we make dist/async.js the main.

$ browserify --standalone async build/index.js | uglifyjs -mc | gzip -9 | wc -c
11089

$ cat dist/async.min.js |  gzip -9 | wc -c
6680
@megawac

This comment has been minimized.

Copy link
Collaborator Author

megawac commented Feb 18, 2016

For the browserify case, it also depends on whether the user is importing
modules which also depend on lodash.

On Thu, Feb 18, 2016 at 2:38 PM, Alex Early notifications@github.com
wrote:

The CJS index seems more "correct", but it would only be better in
node-land. If you're browserify-ing the UMD build will be smaller, and it
will still wok in node. I say we make dist/async.js the main.

$ browserify --standalone async build/index.js | uglifyjs -mc | gzip -9 | wc -c
11089

$ cat dist/async.min.js | gzip -9 | wc -c
6680


Reply to this email directly or view it on GitHub
#996 (comment).

@aearly

This comment has been minimized.

Copy link
Collaborator

aearly commented Feb 18, 2016

If the browserify user was cherry-picking async methods, they'd get some overlap with lodash and some possible de-deuping, if they were also cherry-picking lodash methods. But if you require both async and lodash wholesale, you'll get no module overlap, and some code duplication. It's a tradeoff between code duplication and CJS module overhead.

@jdalton

This comment has been minimized.

Copy link
Contributor

jdalton commented Feb 18, 2016

Just popping in, if you use browserify, are you using bundler-collapser? I know webpack generally produces smaller builds and has more flexibility in terms of module path hackery.

@aearly

This comment has been minimized.

Copy link
Collaborator

aearly commented Feb 18, 2016

$ browserify --standalone async build/index.js | bundle-collapser | uglifyjs -mc | gzip -9 | wc -c
9393

Helps a little. It's also up to the browserify user to use bundle-collapser.

@jtwebman

This comment has been minimized.

Copy link
Contributor

jtwebman commented Feb 19, 2016

I thought browserify was going away. It seems more and more people only are using npm. Maybe that is just from what I have seen. Has anyone else noticed it?

@aearly

This comment has been minimized.

Copy link
Collaborator

aearly commented Feb 19, 2016

Browserify is still alive and well, but whatever the case, here it's just a quick way to get a feel for what the module overhead is, regardless of what bundler you're using.

ecasilla and others added 2 commits Feb 20, 2016
updating the README

fix readme
async.queue.unsaturated #868
@aearly aearly changed the title [WIP] Modularization Modularization Feb 24, 2016
@aearly aearly added the enhancement label Feb 24, 2016
aearly added a commit that referenced this pull request Feb 24, 2016
Modularization
@aearly aearly merged commit 3d1781c into master Feb 24, 2016
3 checks passed
3 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+2.8%) to 100.0%
Details
@megawac

This comment has been minimized.

Copy link
Collaborator Author

megawac commented Feb 24, 2016

When do you want to release v2? and do we want to do a pre-release?

@martinheidegger

This comment has been minimized.

Copy link

martinheidegger commented Feb 24, 2016

I am jumping on this bandwaggon waaay too late but: Why all this effort of splitting it out to own packages? This will make npm just slower. npm supports require('async/something') for files within the async repo. Async only needed to distribute the modules as.js` files in the tree:

- package.json
- parallel.js
- each.js
- serial.js
- ....

Then you could do require('async/parallel') would automatically just load the code required for parallel. This would significantly faster in every respect, wouldn't it?

@megawac

This comment has been minimized.

Copy link
Collaborator Author

megawac commented Feb 24, 2016

Why all this effort of splitting it out to own packages?

We're not doing that at the moment, but the benefit is the download for an individual method is smaller than the entire async package

@jdalton

This comment has been minimized.

Copy link
Contributor

jdalton commented Feb 24, 2016

Though with npm's cache, packages of a given version are only downloaded once.
Subsequent requests hit the local npm cache on your system.

@aearly

This comment has been minimized.

Copy link
Collaborator

aearly commented Feb 24, 2016

I want to release 2.0 when all the issues in the milestone are complete. (All the breaking changes). I'd also like to get the docs migrated to jsdoc and the tests ported to Mocha, but those aren't required.

@martinheidegger

This comment has been minimized.

Copy link

martinheidegger commented Feb 24, 2016

@megawac I do understand the need for splitting out the modules in own files/modules as this will reduce the size if used by packages such as a browserify but when downloading the files through npm this method should slow down things (considering the package.json and other overhead). Browserify (and babel afaik) do support deep binding require("<package>/<module>") so the bundled files would already become a lot smaller.

@megawac

This comment has been minimized.

Copy link
Collaborator Author

megawac commented Feb 24, 2016

rowserify (and babel afaik) do support deep binding require("/") so the bundled files would already become a lot smaller.

Sure, assuming that none of the user's depencies require async and do require("async").

Also, at the moment babel does nothing in regards to import <package>/<module> besides transpile it to require("<package>/<module>"). Bundlers such as webpack, browserify, and rollup do preform this action

@aearly I think it makes sense to set a package.json field for "browser": "index.js", the reason being is webpack and rollup support tree shaking. This would allow the bundler to exclude unused modules (as if the user did require('async/some-module')

@martinheidegger

This comment has been minimized.

Copy link

martinheidegger commented Feb 24, 2016

@megawac Users likely will always be able to do require("async") and blow up their dependencies.

@megawac

This comment has been minimized.

Copy link
Collaborator Author

megawac commented Jul 13, 2016

For reference, I measured the require time of using index.js vs dist/async in v2.

Env dist/async.js async/index.js
v0.12 8 ms 69ms
v4.4.7 9 ms 58ms
v6.2.2 11 ms 46ms

Methodology of test

I used time-require to make these measurements. I ran a tiny script

  require('time-require');

  var async = require('async');
  var async1 = require('async/index');

I ran this in v0.12, v4, and v6 installed via nvm 5 times each and took the average (tended not to deviate much) on my laptop

Computer

Costs involved loading async/index (node 6)

Note: I snipped a couple irrelevant parts of this log

Start time: (2016-07-13 22:37:29 UTC) [treshold=1%]
 #  module                          time  %
 5  ./isFunction (...sFunction.js)   1ms  ▇ 2%
 6  ./toNumber (no.../toNumber.js)   1ms  ▇ 2%
 7  ./toFinite (no.../toFinite.js)   1ms  ▇ 2%
 8  ./toInteger (n...toInteger.js)   1ms  ▇ 2%
 9  lodash/rest (n...dash/rest.js)   2ms  ▇▇ 4%
10  ./internal/app...applyEach.js)   3ms  ▇▇ 6%
11  ./_getLength (...getLength.js)   1ms  ▇ 2%
12  lodash/isArray...ArrayLike.js)   1ms  ▇ 2%
13  ./getIterator...tIterator.js)    1ms  ▇ 2%
14  ./isArray (nod...h/isArray.js)   1ms  ▇ 2%
15  ./_indexKeys (...indexKeys.js)   2ms  ▇▇ 4%
16  lodash/keys (n...dash/keys.js)   2ms  ▇▇ 4%
17  ./iterator (no.../iterator.js)   4ms  ▇▇▇ 8%
18  ./eachOfLimit...chOfLimit.js)    5ms  ▇▇▇ 10%
19  ./internal/doP...llelLimit.js)   6ms  ▇▇▇▇ 12%
20  ./mapLimit (no.../mapLimit.js)   6ms  ▇▇▇▇ 12%
21  ./map (node_mo...async/map.js)   6ms  ▇▇▇▇ 12%
22  ./applyEach (n...applyEach.js)   9ms  ▇▇▇▇▇▇ 18%
23  ./applyEachSer...achSeries.js)   1ms  ▇ 2%
24  ./asyncify (no.../asyncify.js)   1ms  ▇ 2%
25  ./_baseFor (no.../_baseFor.js)   1ms  ▇ 2%
26  lodash/_baseFo...aseForOwn.js)   1ms  ▇ 2%
27  ./auto (node_m...sync/auto.js)   1ms  ▇ 2%
28  lodash/_arrayM..._arrayMap.js)   1ms  ▇ 2%
29  ./_checkGlobal...eckGlobal.js)   1ms  ▇ 2%
30  ./_root (node_...ash/_root.js)   1ms  ▇ 2%
31  ./_Symbol (nod...h/_Symbol.js)   1ms  ▇ 2%
32  ./_baseToStrin...eToString.js)   1ms  ▇ 2%
33  ./_stringToArr...ngToArray.js)   1ms  ▇ 2%
34  lodash/trim (n...dash/trim.js)   2ms  ▇▇ 4%
35  ./autoInject (...utoInject.js)   4ms  ▇▇▇ 8%
36  ./internal/que...nal/queue.js)   1ms  ▇ 2%
37  ./cargo (node_...ync/cargo.js)   2ms  ▇▇ 4%
38  ./eachOfLimit...chOfLimit.js)    1ms  ▇ 2%
39  ./eachOfSeries...hOfSeries.js)   1ms  ▇ 2%
40  ./reduce (node...nc/reduce.js)   1ms  ▇ 2%
41  ./seq (node_mo...async/seq.js)   1ms  ▇ 2%
42  ./compose (nod...c/compose.js)   1ms  ▇ 2%
43  ./internal/doP...oParallel.js)   1ms  ▇ 2%
44  ./concat (node...nc/concat.js)   1ms  ▇ 2%
45  ./internal/doS.../doSeries.js)   1ms  ▇ 2%
46  ./concatSeries...catSeries.js)   1ms  ▇ 2%
47  ./constant (no.../constant.js)   1ms  ▇ 2%
48  lodash/identit.../identity.js)   1ms  ▇ 2%
49  ./internal/cre...ateTester.js)   1ms  ▇ 2%
50  ./detect (node...nc/detect.js)   2ms  ▇▇ 4%
51  ./doDuring (no.../doDuring.js)   1ms  ▇ 2%
52  ./eachLimit (n...eachLimit.js)   1ms  ▇ 2%
53  ./each (node_m...sync/each.js)   1ms  ▇ 2%
54  ./ensureAsync...sureAsync.js)    1ms  ▇ 2%
55  ./filter (node...nc/filter.js)   1ms  ▇ 2%
56  ./forever (nod...c/forever.js)   1ms  ▇ 2%
57  ./mapValues (n...mapValues.js)   1ms  ▇ 2%
58  ./memoize (nod...c/memoize.js)   1ms  ▇ 2%
59  ./internal/par.../parallel.js)   1ms  ▇ 2%
60  ./parallelLimi...llelLimit.js)   1ms  ▇ 2%
61  ./parallel (no.../parallel.js)   1ms  ▇ 2%
62  ./queue (node_...ync/queue.js)   1ms  ▇ 2%
63  ./priorityQueu...rityQueue.js)   1ms  ▇ 2%
64  ./reduceRight...duceRight.js)    2ms  ▇▇ 4%
65  ./reject (node...nc/reject.js)   1ms  ▇ 2%
66  ./rejectSeries...ectSeries.js)   1ms  ▇ 2%
67  ./retryable (n...retryable.js)   1ms  ▇ 2%
68  ./some (node_m...sync/some.js)   1ms  ▇ 2%
69  ./timeout (nod...c/timeout.js)   1ms  ▇ 2%
70  ./timesSeries...mesSeries.js)    1ms  ▇ 2%
71  ./internal/onl...nal/onlyOnce)   1ms  ▇ 2%
72  ./whilst (node...nc/whilst.js)   1ms  ▇ 2%
73  ./until (node_...ync/until.js)   1ms  ▇ 2%
74  async/index (n...ync/index.js)  44ms  ▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇▇ 86%
Total require(): 334
Total time: 51ms

P.s. I hope all your browsers support detail tags :)

Summary
Computer
Processor4x Intel(R) Core(TM) i5-4300M CPU @ 2.60GHz
Memory11988MB (7240MB used)
Operating SystemUbuntu 14.04.4 LTS
@aearly

This comment has been minimized.

Copy link
Collaborator

aearly commented Jul 13, 2016

Cool! Thanks for backing up my gut assertions with some actual data. 😄

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.