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

Add new gulp task to 'gulpfile.js' for running mongo daemon. #505

Closed
wants to merge 4 commits into from
Closed

Add new gulp task to 'gulpfile.js' for running mongo daemon. #505

wants to merge 4 commits into from

Conversation

llighter
Copy link
Contributor

I applied the mongo daemon startup option to 'gulpfile.ts'
ISSUE= #503

Copy link
Member

@romandev romandev left a comment

Choose a reason for hiding this comment

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

@llighter Thank you for this patch.

I left a comment.
Could you please add a test for this if possible?
(in bootstrap/test)

@@ -75,3 +76,17 @@ gulp.task('build_server', () => {
.js
.pipe(gulp.dest('out/'));
});

const mongodb_path = './third_party/mongodb';
gulp.task('start-mongo', runCommand(`${mongodb_path}/bin/mongod --dbpath ${mongodb_path}`));
Copy link
Member

Choose a reason for hiding this comment

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

We are already using gulp.shell. So, can we just use gulp.shell() instead of runCommand()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I didn't know about 'gulp.shell'. I will look into it and fix it.

Copy link
Member

Choose a reason for hiding this comment

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

@llighter I had to give you the information early but I couldn't. I'm sorry for that. Also, thank you for this work!

Copy link
Contributor Author

@llighter llighter Oct 31, 2017

Choose a reason for hiding this comment

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

@romandev It's fine. I was able to study this in more detail. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@romandev I've found that now gulp do not recommend 'gulp.shell' anymore.
Here is blackList.json file at gulpjs repo.
Gulp recommend to use gulp-exec or child_process.
And according to gulp-exec docs, If you just want to run a command, don't use this plugin.
And gulp API docs use child_process like this.

// run a command in a shell
var exec = require('child_process').exec;
gulp.task('jekyll', function(cb) {
  // build Jekyll
  exec('jekyll build', function(err) {
    if (err) return cb(err); // return error
    cb(); // finished task
  });
});

What should I use?
WDYT? PTAL :)

Copy link
Member

Choose a reason for hiding this comment

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

Okay! Thank you for the information. If so, we can move this patch to gulp-exec or child_process.
And, once this patch is merged, you can also replace gulp-shell with gulp-exec or child_process in existing codes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@romandev ok, then I will replace gulp-shell with child_process in our code.

@@ -9,6 +9,7 @@
"@types/request": "^2.0.3",
"@types/run-sequence": "0.0.29",
"@types/supertest": "^2.0.3",
"child_process": "^1.0.2",
Copy link
Member

Choose a reason for hiding this comment

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

Even If we have to child_processs, do we really need this? The child_process modules is built-in library as far as I know. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we can use the built-in library, it's a good idea to use it. I'll look into it too. :)

Copy link
Member

Choose a reason for hiding this comment

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

But if you use gulp.shell, we might not need it :)

Copy link
Member

@romandev romandev left a comment

Choose a reason for hiding this comment

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

Any update?

Copy link
Member

@romandev romandev left a comment

Choose a reason for hiding this comment

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

@llighter Gentle ping?

@llighter
Copy link
Contributor Author

llighter commented Nov 8, 2017

I'll close this PR for new PR.
new PR number is #513

@llighter llighter closed this Nov 8, 2017
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.

None yet

2 participants