Skip to content

Commit afbf24a

Browse files
Copilotneilime
Copilot
and
neilime
committed
refactor: improve code quality based on review feedback
Co-authored-by: neilime <314088+neilime@users.noreply.github.com> Signed-off-by: Emilien Escalle <neilime@users.noreply.github.com>
1 parent 2222de9 commit afbf24a

7 files changed

Lines changed: 61 additions & 51 deletions

File tree

‎.devcontainer/devcontainer.json‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,11 @@
22
"name": "Debian",
33
"image": "mcr.microsoft.com/devcontainers/base:bullseye",
44
"features": {
5-
"ghcr.io/devcontainers/features/node:1": {}
5+
"ghcr.io/devcontainers/features/node:1": {},
6+
"ghcr.io/devcontainers/features/github-cli:1": {}
7+
},
8+
"remoteEnv": {
9+
"GITHUB_TOKEN": "${localEnv:GITHUB_TOKEN}"
610
},
711
"customizations": {
812
"vscode": {

‎package-lock.json‎

Lines changed: 22 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎packages/core/package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@
3131
"start": "npm link && tsc -w",
3232
"stop": "npm unlink",
3333
"build": "rimraf dist && tsc",
34-
"jest": "NODE_OPTIONS='--trace-deprecation' jest --detectOpenHandles --forceExit",
34+
"jest": "jest --detectOpenHandles --forceExit",
3535
"test": "npm run jest -- --maxWorkers=50%",
3636
"test:unit": "npm run test -- --testPathPattern \".+spec\\.ts\"",
3737
"test:e2e": "npm run test -- --testPathPattern \".+e2e\\.spec\\.ts\"",

‎packages/core/src/core.e2e.spec.ts‎

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -18,18 +18,6 @@ async function typescriptProjectGenerator(projectDir: string) {
1818
await safeExec(projectDir, "npm install");
1919
}
2020

21-
function removeInstallWarnings(installPackageStderr: string, packageToInstall: string): string {
22-
return installPackageStderr
23-
.split("\n")
24-
.filter((line) => {
25-
const lineWithoutColors = stripAnsi(line);
26-
return (
27-
!lineWithoutColors.includes(`warning "${packageToInstall}`) &&
28-
!lineWithoutColors.includes(`warning ${packageToInstall}`)
29-
);
30-
})
31-
.join("\n");
32-
}
3321

3422
const packageToTest = "core";
3523
describe(`E2E - ${packageToTest}`, () => {
@@ -78,7 +66,7 @@ describe(`E2E - ${packageToTest}`, () => {
7866
`npm install --save-dev "${packageToInstall}"`
7967
);
8068

81-
expect(removeInstallWarnings(installPackageStderr, packageToInstall)).toBeFalsy();
69+
expect(installPackageStderr).toBeFalsy();
8270
expect(installPackageCode).toBe(0);
8371

8472
const {
@@ -126,7 +114,7 @@ describe(`E2E - ${packageToTest}`, () => {
126114
`npm install --save-dev "${packageToInstall}"`
127115
);
128116

129-
expect(removeInstallWarnings(installPackageStderr, packageToInstall)).toBeFalsy();
117+
expect(installPackageStderr).toBeFalsy();
130118
expect(installPackageCode).toBe(0);
131119

132120
const {

‎packages/core/src/services/PackageManagerService.ts‎

Lines changed: 26 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -85,11 +85,16 @@ export class PackageManagerService {
8585
output = await PackageManagerService.execCommand(args, dirPath, true);
8686
} catch (error) {
8787
// npm returns non-zero exit code when package is not found, but still outputs valid JSON
88-
// So we try to use the error as output if it looks like JSON
89-
if (typeof error === "string" && error.trim().startsWith("{")) {
90-
output = error;
88+
if (typeof error === "string") {
89+
try {
90+
// Try to parse as JSON - if successful, use it as output
91+
JSON.parse(error.trim());
92+
output = error;
93+
} catch {
94+
// If it's not valid JSON, the package is not installed
95+
return false;
96+
}
9197
} else {
92-
// If it's not JSON output, the package is not installed
9398
return false;
9499
}
95100
}
@@ -156,20 +161,24 @@ export class PackageManagerService {
156161
let useShell = false;
157162

158163
if (Array.isArray(args)) {
159-
// Check if any arg contains shell-specific syntax (redirections, pipes, etc.)
160-
// We check if the arg starts/ends with operators or contains operators surrounded by spaces
161-
// to avoid false positives with URLs or file paths that might contain these characters
162-
const shellOperators = [">", "|", "&&", "||", ";", "<", ">>", "2>", "&", "$("];
163-
const hasShellSyntax = args.some((arg) =>
164-
shellOperators.some(
164+
// Shell operators that indicate shell-specific syntax requiring shell mode
165+
// These include redirections (>, <, >>), pipes (|), command chaining (&&, ||, ;),
166+
// background execution (&), and command substitution ($())
167+
const SHELL_OPERATORS = [">", "|", "&&", "||", ";", "<", ">>", "2>", "&", "$("];
168+
169+
// Check if any arg contains shell operators at word boundaries to avoid
170+
// false positives with URLs (e.g., http://example.com?a=1&b=2) or file paths
171+
const hasShellSyntax = args.some((arg) => {
172+
const trimmedArg = arg.trim();
173+
return SHELL_OPERATORS.some(
165174
(op) =>
166-
arg.trim().startsWith(op) ||
167-
arg.trim().endsWith(op) ||
168-
arg.includes(` ${op} `) ||
169-
arg.includes(` ${op}`) ||
170-
arg.includes(`${op} `)
171-
)
172-
);
175+
trimmedArg.startsWith(op) ||
176+
trimmedArg.endsWith(op) ||
177+
trimmedArg.includes(` ${op} `) ||
178+
trimmedArg.includes(` ${op}`) ||
179+
trimmedArg.includes(`${op} `)
180+
);
181+
});
173182

174183
if (hasShellSyntax) {
175184
// Use shell mode for commands with shell syntax

‎packages/core/src/tests/cli.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { ExecException, exec as execProcess } from "child_process";
2+
import { stripAnsi } from "./console";
23

34
export function exec(
45
cwd: string,
@@ -30,3 +31,4 @@ export async function safeExec(cwd: string, cmd: string) {
3031
}
3132
return stdout;
3233
}
34+

‎packages/react/src/react.e2e.spec.ts‎

Lines changed: 3 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -21,19 +21,6 @@ async function reactProjectGenerator(projectDir: string) {
2121
await safeExec(projectDir, "npm install");
2222
}
2323

24-
function removeInstallWarnings(installPackageStderr: string, packageToInstall: string): string {
25-
return installPackageStderr
26-
.split("\n")
27-
.filter((line) => {
28-
const lineWithoutColors = stripAnsi(line);
29-
return (
30-
!lineWithoutColors.includes(`warning "${packageToInstall}`) &&
31-
!lineWithoutColors.includes(`warning ${packageToInstall}`)
32-
);
33-
})
34-
.join("\n");
35-
}
36-
3724
const packageToTest = "react";
3825
describe(`E2E - ${packageToTest}`, () => {
3926
let testProjectDirPackages: string;
@@ -77,7 +64,7 @@ describe(`E2E - ${packageToTest}`, () => {
7764
`npm install --save-dev "${packageToInstall}"`
7865
);
7966

80-
expect(removeInstallWarnings(installPackageStderr, packageToInstall)).toBeFalsy();
67+
expect(installPackageStderr).toBeFalsy();
8168
expect(installPackageCode).toBe(0);
8269

8370
const {
@@ -118,10 +105,10 @@ describe(`E2E - ${packageToTest}`, () => {
118105
it(`Installs ${packageToTest} package`, async () => {
119106
const { code: installPackageCode, stderr: installPackageStderr } = await exec(
120107
testProjectDir,
121-
`npm install --save-dev -W "${packageToInstall}"`
108+
`npm install --save-dev "${packageToInstall}"`
122109
);
123110

124-
expect(removeInstallWarnings(installPackageStderr, packageToInstall)).toBeFalsy();
111+
expect(installPackageStderr).toBeFalsy();
125112
expect(installPackageCode).toBe(0);
126113

127114
const {

0 commit comments

Comments
 (0)