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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

feat(create-waku): install dependencies automatically when creating a new waku #728

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b915f15
feat: install dependencies automattically when creating the waku project
Rec0iL99 May 28, 2024
55e5206
dynamic package manager
Rec0iL99 May 28, 2024
26eb8ba
removed cross-spawn and used spawn from node:child_process
Rec0iL99 Jun 25, 2024
6e1dddf
process is a global env
Rec0iL99 Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 32 additions & 6 deletions packages/create-waku/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
parseExampleOption,
downloadAndExtract,
} from './helpers/example-option.js';
import { spawn } from 'node:child_process';

const userAgent = process.env.npm_config_user_agent || '';
const packageManager = /pnpm/.test(userAgent)
Expand Down Expand Up @@ -202,15 +203,40 @@ async function init() {
await installTemplate(root, packageName, templateRoot, templateName);
}

// TODO automatically installing dependencies
// 1. check packageManager
// 2. and then install dependencies

console.log(`\nDone. Now run:\n`);
console.log(`${bold(green(`cd ${targetDir}`))}`);
console.log(`${bold(green(commands.install))}`);
console.log(`${bold(green(commands.dev))}`);
console.log();
console.log(`Installing dependencies by running ${commands.install}...`);

process.chdir(targetDir);

const installProcess = spawn(packageManager, ['install']);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const installProcess = spawn(packageManager, ['install']);
const installProcess = spawn(packageManager, ['install'], { encoding: 'utf8' });

Copy link
Contributor Author

@Rec0iL99 Rec0iL99 Jun 26, 2024 •
edited
Loading

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.

Copy link
Member

Choose a reason for hiding this comment

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

first to know this

Copy link
Member

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);
Copy link
Member

Choose a reason for hiding this comment

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

#728 (comment)

Does it show the indicator as expected? cc @ojj1123

Copy link
Contributor

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!

Copy link
Member

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.

Copy link
Contributor

@ojj1123 ojj1123 Jul 1, 2024 •
edited
Loading

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) => {

Copy link
Contributor

@ojj1123 ojj1123 Jul 1, 2024 •
edited
Loading

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)

스크린샷 2024-07-01 19 23 18

});

installProcess.stderr.setEncoding('utf8');
installProcess.stderr.on('data', (data) => {
console.log(data);
});

installProcess.on('close', (code) => {
// process exit code
if (code !== 0) {
console.error(`Could not execute ${commands.install}. Please run`);
console.log(`${bold(green(`cd ${targetDir}`))}`);
console.log(`${bold(green(commands.install))}`);
console.log(`${bold(green(commands.dev))}`);
console.log();
} else {
console.log(`\nDone. Now run:\n`);
console.log(`${bold(green(`cd ${targetDir}`))}`);
console.log(`${bold(green(commands.dev))}`);
console.log();
}
});
}

init()
Expand Down
Loading