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

suggestion: internally split into separate module files to make optimised builds easier #720

Closed
jokeyrhyme opened this issue Feb 26, 2015 · 22 comments

Comments

@jokeyrhyme
Copy link

I just forked this project specifically so I could use priorityQueue() and virtually nothing else. It occurs to me that this is a long-term mistake because I'll miss out on future improvements to this function unless I manually import them.

May I suggest splitting async.js into multiple files in a lib sub-directory, to facilitate delivering async's functionality to the browser whilst eschewing unnecessary bytes?

This may be of little benefit for server-side users, but every byte counts in the front-end.

Also, whilst undertaking this pruning, it occurred to me that dependencies between internal and exported methods isn't always clear unless you carefully read the source code. Modularising could assist in making these interdependencies more clear.

What do you think? Should I start a Pull Request for this, or is a bad idea / waste of time?

@jokeyrhyme
Copy link
Author

PS. my priorityQueue() fork is here: https://github.com/blinkmobile/async-priority-queue

@jokeyrhyme
Copy link
Author

What I'd be aiming for is being able to do something like

var priorityQueue = require("async/lib/priority-queue");

@andyburke
Copy link

I came here looking for this issue. I am in the same boat: I use a handful of async methods, but I don't need the whole thing. It would be great if these could be split out.

I am tempted to create a PR that splits everything. Any feedback on if that would be well received or not?

@jokeyrhyme
Copy link
Author

@caolan ?

@aearly
Copy link
Collaborator

aearly commented Mar 5, 2015

This is an often requested feature. There are advantages to having it both ways, as a single file, or a collection of files. Ease of use vs. modularity. We could author as a multiple modules, but then still publish a single file with browserify --standalone in a prepublish script.

We would still have to check that single built file in order to support people using the whole shebang with Bower. There would also be the problem of people submitting PR's to the built file, rather than the modules...

There is also the lodash-cli style route. Author a single file, but create a tool to split things up and create custom builds. This is a lot of work -- the SLOC of lodash-cli rivals that of lodash itself (with more complexity). lodash-cli has only succeeded because jdalton works on lodash nearly full time.

Whatever the case, don't go creating a PR with everything split out without coordination with @caolan . If you did split things up, it would invalidate a bunch of outstanding PRs waiting to be merged.

@andyburke
Copy link

I spent a couple hours (because I was interested, not looking to cause any bad blood) trying to break async up into smaller modules. Here's the result:

https://github.com/andyburke/antisync

It passes all the tests except for noConflict (I removed that in preference to simplifying to only support node/browserify since I have no real knowledge of the other package managers or what's involved in testing them).

A full browserify build of this modular version is ~3k larger than an async build (due to the overhead of more files, more require statements, etc).

However, in a real-life use case where I was only using series+parallel+waterfall, antisync ended up about 1/3 the size of async.

After minification, antisync ended up saving about 10k on the minified bundle I am currently using async in.

I'm not sure what my next steps would be. I don't really fancy maintaining a fork of async, but there are things about this approach that I really prefer to the monolithic setup that currently exists.

One thing I would like to improve, but I'm not really sure there's a way is that the root of antisync is full of module files and it's a bit noisy. I did that to support sane require()s, eg:

var antisync = {
    series: require( 'antisync/series' ),
    parallel: require( 'antisync/parallel' )
};

I'm turned off by the idea of requires that would look like this (note the /lib/ in the middle):

var antisync = {
    series: require( 'antisync/lib/series' )
};

... that seems a lot less intuitive. If anyone has suggestions on how to address that, I'd be all ears. (I wasn't able to find anything in package.json docs to allow for hoisting a directory into the root include path.)

Thoughts?

@aearly
Copy link
Collaborator

aearly commented Mar 5, 2015

Seems like you have a solution that works for you for now.

There's nothing wrong with having a bunch of js files at the root. You could move some of the non-public utility methods to lib/.

bundle-collapser will also help you get the browserify build smaller.

@dansilivestru
Copy link

This seems like a very useful change.

We use async very heavily and having the project be more modular would be a great help on many fronts. First off, @andyburke's changes have really improved the overall health and maintainability of the asyc project as seen here (disclaimer, I work at bitHound):

https://www.bithound.io/github/andyburke/antisync and here's the impact of the main refactoring commit: https://www.bithound.io/github/andyburke/antisync/commit/317a8e5a5aeda6d0374ccca59009e14e389f7a4d

I think a change like this could greatly benefit the project for a few reasons:

  • Easier to maintain, since changes/additions can now be restricted to the affected module
  • Increased contributions, due to it being easier to understand where changes need to be made
  • Pull requests would be greatly simplified and therefore can be pulled in quicker
  • Provide the ability to use async in a more modular way, based on a consumer's individual needs

As mentioned above, there are some negatives as well:

  • A build step is being introduced
  • This would negatively impact existing pull requests (temporary problem)
  • Final async.js file size is larger

We love this project and would be very happy to contribute to help make this happen. We can help complete the work and would also be happy to convert any existing pull requests to fit the new project structure.

Would there be interest in having @andyburke issue an initial pull request and having us help bring the pull request to a state that would be acceptable to this project? Including updating any existing pull requests that would need to be updated?

I would love to hear @caolan comments and thoughts on this.

@jokeyrhyme
Copy link
Author

A build step is being introduced

One proposed solution (if it was important to leave lib/async.js where it is):

  • configure the build process to output to lib/async.js
  • continue to version and distribute lib/async.js

So existing consumers don't experience any change.
Node.js and Browserify consumers can optionally require specific sub-paths to optimise further.

@aearly
Copy link
Collaborator

aearly commented Mar 31, 2015

We also would have to keep Bower users happy. We might have to bundle on a Bower pre-install (or equivalent hook). As of right now they can just use the single async.js file directly from git without us having to explicitly support bower.

@andyburke
Copy link

If I added a step to build lib/async.js, probably using browserify (and maybe collapser to help with size), would that make this an easier change to digest? (I won't have time to get to this until next week at the earliest, but I would welcome a PR off my branch.)

I'm not sure I understand the bower issue: if I built to lib/async.js using browserify with an 'async' export, would it just work for them or is there something else to it?

I am also not sure how to handle the noConflict thing (or really what the purpose of that is)... any recommendations?

@aearly
Copy link
Collaborator

aearly commented Mar 31, 2015

browserify --standalone will do most of what we would need. async.noConflict() actually becomes simpler -- previous_async just becomes global.async.

The bower stuff is because people like to use async either as a global or with AMD. It would be best to have it as a bower build step so we don't have to check in a generated file (and confuse people as to where to submit pull requests).

@gtanner
Copy link

gtanner commented Apr 2, 2015

I added a pull request to antisync to add the build step. andyburke#1

@andyburke
Copy link

Thanks @gtanner, merged.

@andyburke
Copy link

@aearly Do you feel like the steps you've listed, if taken, would make my changes more palatable to merge? Specifically, it seems like you're requesting:

  1. Ensure we can build the standalone lib
  2. Verify that noConflict is eliminated or simplified
  3. Ensure that bower is supported and generates the standalone lib as a build step

Do you have commit privileges or is @caolan the final arbiter?

@jokeyrhyme
Copy link
Author

You don't need to have bower execute the build step if we just continue to commit the build output to this git repository. There ought to be zero impact for bower users then.

@aearly
Copy link
Collaborator

aearly commented May 7, 2015

I don't have commit privileges, I just help out with issue triage. (But it might be worth asking if he's open for more maintainers.)

Those three steps sound good for a change like this. But I would still prefer this happen after the backlog of pull-requests is dealt with.

@aearly aearly added the feature label May 19, 2015
@justincy
Copy link
Contributor

justincy commented Jun 2, 2015

@aearly Do you have commit privileges now?

How can we move this along? It needs to happen. I'm willing to help.

@aearly
Copy link
Collaborator

aearly commented Jun 2, 2015

I don't want to think about this just yet -- still wrangling lots of issues and PRs. Right now I'm leaning towards the lodash-cli strategy, where we parse out the modules programmatically, leaving a lean source file people can use wholesale. There are a lot of modules that would end up being a single line, which would add a lot of overhead and make refactoring more difficult.

@gmaclennan
Copy link
Contributor

Just to add my voice to the many people requesting this. I don't know if modules being a single line is so much of an issue. How much overhead would it actually add? As for refactoring and maintainability, I'll leave that up to whoever will be maintaining, but being able to require just the pieces I need would be a huge win for me.

@aearly
Copy link
Collaborator

aearly commented Jun 15, 2015

A lot of the async's methods are generated, especially the foo fooSeries fooLimit variants, with more and more being defined in terms of other async methods. If we modularized each method manually, it makes changing how methods are generated more cumbersome, having to swap out require()s and create new .js files, etc.. PRs to a single file are also easier to manage -- less chance of conflicts if we happened to change how things are structured.

It also means for the browser, many modules would be a single line with a handful of require() statements, plus the overhead of a wrapper function. Even with something like bundle-collapser, this still adds 100-200 bytes per module, which across 70 methods could add up to 15kB of overhead. async is 35kb today.

I really want to go the lodash-cli route where we parse out each module programmatically, meaning we can author async as a single file, but publish as both a monolith and system of modules to keep both types of async module users happy. I haven't had time to fully evaluate that strategy though, and it's hard to get a big picture out of lodash-cli's 3000 sloc main file. @jdalton over in #748 seems confident we can do it, if we work on it incrementally.

When I have time, I'm going to make an experimental async-cli project (and maybe help tame lodash-cli to get a better feel for it). Collaborators will be welcome.

@aearly
Copy link
Collaborator

aearly commented Jan 7, 2016

Closing this, we're working on this in #996.

@aearly aearly closed this as completed Jan 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants