Skip to content

feat(async): all commands have async versions#387

Closed
nfischer wants to merge 1 commit intomasterfrom
async-commands
Closed

feat(async): all commands have async versions#387
nfischer wants to merge 1 commit intomasterfrom
async-commands

Conversation

@nfischer
Copy link
Copy Markdown
Member

@nfischer nfischer commented Mar 8, 2016

Fixes #2. This is an initial stab at async-ing everything. not ready to merge, since docs haven't been updated.

I think this is a nice interface, since you can specify commands to be async either with:

cp('file1.txt', 'out1.txt'); // synchronous, like normal
cp('file2.txt', 'out2.txt', {async: true}); // asynchronous, no callback
cp('file3.txt', 'out3.txt', {async: true}, function(code, stdout, stderr) { // asynchronous, with callback
  console.log(code);
  // ...
});
cp('file4.txt', 'out4.txt', function(code, stdout, stderr) { // callback implies asynchronous
  console.log(code);
  // ...
});

This is consistent with the precedent set by exec().

An alternative is to pass in the actual return value (usually a ShellString). This is better for using with pipes. I think the following is a suitable workaround, however, with this implementation:

grep('largefile.txt', function(code, stdout, stderr) {
  var ret = ShellString(stdout, stderr);
  ret.sed(/o/g, 'a');
});

retValue = fn.apply(this, args);
if (config.async || callback) {
process.nextTick(function() {
var r = fn.apply(this, args);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't really work, because if the function itself blocks (ie. it uses blocking APIs), then this will too.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would this not work? The function won't be called until everything on the event loop is dealt with, right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but imagine this scenario:

setTimeout(() => console.log('It\'s been 1 second!'), 1000);
exec('sleep 10; echo "It\'s been 10 seconds!"', function noop() {}, { async: true });

Outputs:

It's been 10 seconds.
It's been 1 second.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ariporad I just tried this example. I saw:

It's been 1 second!
It's been 10 seconds!

Also, exec()'s async version is totally different than the rest of the commands.

Another example:

> setTimeout(() => console.log('It\'s been 1 second!'), 5000);
> ls({async: true}, function() { echo('hi'); });

Output (as I would expect, since ls() is super fast) :

hi
It's been 1 second!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nfischer: Whoops, my bad.

Try this (I know it's unrealistic times, but for argument's sake):

function sleep(time) {
    var start = Date.now()
    while (start + time > Date.now()) ;
};

setTimeout(() => console.log('1 sec'), 1000);
sleep(2000);
console.log('2 sec');

Expected:

1 sec
2 sec

Actual:

2 sec
1 sec

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ariporad your last example uses no shelljs. the actual output is what a JS programmer ought expect: you have to yield execution.

@nfischer nfischer closed this Jun 26, 2018
@nfischer nfischer deleted the async-commands branch July 4, 2018 00:36
@nfischer nfischer restored the async-commands branch July 4, 2018 00:36
@nfischer nfischer mentioned this pull request Jul 4, 2018
@nfischer nfischer deleted the async-commands branch March 8, 2025 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants