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

Don't assume shell.exec() is synchronous #95

Closed
vthunder opened this issue Jan 29, 2015 · 2 comments · May be fixed by SibuStephen/mean-cli#33 or OriPekelman/mean-cli#44
Closed

Don't assume shell.exec() is synchronous #95

vthunder opened this issue Jan 29, 2015 · 2 comments · May be fixed by SibuStephen/mean-cli#33 or OriPekelman/mean-cli#44

Comments

@vthunder
Copy link

While investigating #94 I noticed that in some places there is an assumption that shell.exec() is synchronous. In fact, it is not when a callback is provided:

https://github.com/arturadib/shelljs#execcommand--options--callback

In this case, for example, init() will return immediately since shell.exec() is the last statement in the function. It's not clear whether init() is expected to fully complete its work before returning:

https://github.com/linnovate/mean-cli/blob/master/lib/cli.js#L896

I've no evidence this is a problem right now, but if the caller does expect init to be finished upon returning, then script could conceivably exit before the callback has a chance to run. Either init() should explicitly return a promise, take a callback, or ensure it completes its work before returning.

In this case, assuming we want the latter, simply adding {async:false} should do the trick.

@andrija-hers
Copy link
Contributor

Great piece of information here, 👍 !

@liorkesos
Copy link
Member

#94 is closed. thanks for the info here.

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