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

Execute callback when smoosh has completed or between commands #13

Merged
merged 5 commits into from
Jan 15, 2012
Merged

Execute callback when smoosh has completed or between commands #13

merged 5 commits into from
Jan 15, 2012

Conversation

tauren
Copy link
Contributor

@tauren tauren commented Oct 17, 2011

This pull request addresses issue #12. It adds support for a done command that can execute a callback. It is intended to be used like this:

smoosh
  .config('smoosh.json')
  .clean()
  .run()
  .build()
  .done( function () {
    console.log ('Smoosh is done');
  });

Note that the done command can be added multiple times between other commands as well:

smoosh
  .config('smoosh.json')
  .clean()
  .run()
  .done( function () {
    console.log ('Smoosh has run');
  })
  .build()
  .done( function () {
    console.log ('Smoosh is done');
  });

The command could be named callback or something else besides done if you prefer. I've updated the README.md file to include documentation as well.

Implementation Details

I went through everything that smoosh does and discovered it uses sync methods for almost everything. This includes using readFileSync and writeFileSync. All of the tools it depends on are used in synchronous ways, including uglifyjs, jshint, gzip, rimraf, and sqwish. This is what enables the commands to be chained like they are.

This means, for instance, that when build is executed, everything in run has already completed. So when done is executed, we know that all of the step prior to it have already finished.

However, there are two dependencies that were causing problems, and that is asciimo and gzip. This is because they runs asynchronously. All the asciimo dependency does is create the SMOOSH banner, which really isn't critical to the operation of Smoosh.

The smoosh output was being rendered to the console in a manner that I think could be problematic. Inside of config(), the smoosh banner was created asynchronously. Once it was finished being created, then all of the output from smoosh was flushed to console.out. But what if the chained commands haven't finished running yet and have not added all the messages to the output queue? It seems to me like there could be a race condition here. If the generating of the smoosh banner took less time than the running, building, and anlayzing, I'm thinking that some of the output wouldn't have been displayed.

The solution I went with was to add _write.flush() just before each command returns, so each command is responsible for flushing it's own output. I also removed the use of asciimo since the async nature of it would have made it much more difficult to add a callback feature to smoosh. Instead, the smoosh banner is simply hard coded.

There is still one minor flaw in this solution. The gzip problem isn't as easy to solve without using something like async method queues[1] by @ded. Fortunately, gzip is only used in analyze, which no other commands rely on. It does mean that the gzip output from the analyze command may be displayed after a callback has been run. For now, the fix I've proposed will functionally work, but some output might be displayed late.

To keep asciimo or to get analyze with gzip working perfectly, then it will be much more complex to support asynchronous commands with callbacks while using chaining. At that point I'd recommend using @ded's solution. But it seems to me like overkill just to be able to use asciimo.

[1] http://www.dustindiaz.com/async-method-queues

@danro
Copy link

danro commented Jan 14, 2012

I wrote a build script this week that needed this functionality, thanks for writing it @tauren! Seems to work in my limited tests .. which mainly consist of config(conf).build("compressed").done(function () {});

@tauren
Copy link
Contributor Author

tauren commented Jan 14, 2012

@danro - glad it worked for you!

@fat - any chance this could be pulled into master soon so I don't have to maintain a separate fork of smoosh? Is there something in the patch that you object to?

fat added a commit that referenced this pull request Jan 15, 2012
Execute callback when smoosh has completed or between commands
@fat fat merged commit 5e5a0c4 into fat:master Jan 15, 2012
@fat
Copy link
Owner

fat commented Jan 15, 2012

thanks - sorry i had missed this

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

Successfully merging this pull request may close these issues.

3 participants