- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 142
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
feat(create-waku): install dependencies automatically when creating a new waku #728
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
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.
Hi, thanks for working on it! Please see comments.
cc @ojj1123 would you like to give a review?
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.
Good works! I review for something.
@Rec0iL99 Are you still around? |
|
||
process.chdir(targetDir); | ||
|
||
const installProcess = spawn(packageManager, ['install']); |
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.
const installProcess = spawn(packageManager, ['install']); | |
const installProcess = spawn(packageManager, ['install'], { encoding: 'utf8' }); |
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.
Hi @himself65, encoding
is not a property that exists on the options object for spawn
.
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.
first to know this
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.
Oh, I think if you just wanna bypass the output, use stdio: 'inherit'
|
||
installProcess.stdout.setEncoding('utf8'); | ||
installProcess.stdout.on('data', (data) => { | ||
console.log(data); |
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.
Does it show the indicator as expected? cc @ojj1123
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.
Yes. I could see the indicator as expected!
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.
#728 (comment) I wonder if stdio: inherit works.
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.
The docs is explaining a difference between stdio: "inherit"
and stdio: "pipe"
but it's a bit confusing for me.
I've tried to use stdio: "inherit"
, it could also show the loading indicator.
stdio: "pipe"
(default)
- child stdios(stdin, stdout, stderr) and parent ones are mapped(piping) each other. But if we want to access the data, we have to use the events like this:
childProcess.stdout.on("data", (data) => {
console.log(data)
})
stdio: "inherit"
- parent's stdios are handed off to the child process. That is, child process could use parent's stdios. So we don't need to use the events.
I think no matter we use stdio: "pipe"
or stdio: "inherit"
. But if we use "inherit", we don't need to use the events:
diff --git a/packages/create-waku/src/index.ts b/packages/create-waku/src/index.ts
index 1eb01d3..98ede50 100644
--- a/packages/create-waku/src/index.ts
+++ b/packages/create-waku/src/index.ts
@@ -210,16 +210,8 @@ async function init() {
process.chdir(targetDir);
- const installProcess = spawn(packageManager, ['install']);
-
- installProcess.stdout.setEncoding('utf8');
- installProcess.stdout.on('data', (data) => {
- console.log(data);
- });
-
- installProcess.stderr.setEncoding('utf8');
- installProcess.stderr.on('data', (data) => {
- console.log(data);
+ const installProcess = spawn(packageManager, ['install'], {
+ stdio: 'inherit',
});
installProcess.on('close', (code) => {
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.
+) If we use stdio: "inherit"
(or anything other than "pipe"), childProcess's stdin
, stdout
and stderr
could be null
:
const installProcess = spawn(packageManager, ['install'], {
stdio: 'inherit',
});
installProcess.stdout // it could be null so we need to null-check
installProcess.stdout.on("data", callbackFn)
|
||
process.chdir(targetDir); | ||
|
||
const installProcess = spawn(packageManager, ['install']); |
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.
Oh, I think if you just wanna bypass the output, use stdio: 'inherit'
@Rec0iL99 Are you still around? |
If @Rec0iL99 is busy and there is no response, it's okay for me to get this work. |
@ojj1123 Yeah, please go ahead. |
This comment was marked as outdated.
This comment was marked as outdated.
Oh it does NOT show the loading indicator on installing. I got mistakes. 😅 I couldn't push my commit to this PR, so I made a new PR 👉 #808 |
#808 is merged. |
… new waku project (#808) follow up wakujs/waku#728 - [x] implement a new feature --------- Co-authored-by: Joel Mathew Koshy <joelmathewkoshy@gmail.com> Co-authored-by: Daishi Kato <dai-shi@users.noreply.github.com>
… new waku project (#808) follow up wakujs/waku#728 - [x] implement a new feature --------- Co-authored-by: Joel Mathew Koshy <joelmathewkoshy@gmail.com> Co-authored-by: Daishi Kato <dai-shi@users.noreply.github.com>
Workflow: