-
-
Notifications
You must be signed in to change notification settings - Fork 35
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
implement sandboxing for js tasks #42
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Charlike Mike Reagent <olsten.larck@gmail.com>
Signed-off-by: Charlike Mike Reagent <olsten.larck@gmail.com>
@@ -63,24 +63,26 @@ class Maid { | |||
throw new MaidError(`Task '${task.name}' failed.\n${err.stack}`) | |||
} | |||
if (checkTypes(task, ['sh', 'bash'])) { | |||
return runCLICommand({ task, resolve, reject }) | |||
runCLICommand({ task, resolve, reject }) | |||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why split the return
statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because it's more meaningful and it may be a bit confusing. we dont return promise from there anyway. and we dont need to return from there either because we are passing resolve and reject to the runCLICommand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, return frome promise constructor make no sense, you are forced to use resolve and reject anyway. only if something fail in that constructor only then it goes to the catch handler, otherwise you are forced to use resolve and reject
in this case it will just throw , because we directly await the promise
I always thought that it's always a good practice to separate the return inside callbacks and always did that in the old days of callbacks. It just not make any sense. The promise constructor is a callback. |
fixes #6 -
lib/runJavaScript.js
is pretty cool, we should publish it to npm ;d I didn't found such thing, but i'm 99% that i seen such modules in the past year.And actually, i don't see how we can implement #11, even if we not merge this PR.