feat: friendlier error message when command not found #6887

KSXGitHub · 2023-08-01T10:03:10Z

Related comment: scaffold-eth/scaffold-eth-2#444 (comment)

Initially, I have hoped that I can create a hint for pnpm $script in general, but since I don't know how to distinguish between pnpm $script and pnpm exec $script, I have to settle for pnpm exec $script only.

zkochan · 2023-08-01T10:29:25Z

exec/plugin-commands-script-runners/src/exec.ts

+            const nearestCommand = getNearestCommand(params[0], (await readProjectManifestOnly(opts.dir)).scripts)
+            if (nearestCommand) {
+              err.hint = `Did you mean "pnpm exec ${nearestCommand}"?`


This doesn't make sense. The pnpm exec command is not for running scripts. pnpm exec is for running binaries from node_modules/.bin or just running any commands. For running scripts there is the pnpm run command.

resolves #6887 (comment)

KSXGitHub · 2023-08-01T11:57:22Z

@zkochan Ideally, if there is a way to distinguish between pnpm $command and pnpm exec $command, I can create 2 hints that work for each case.

zkochan · 2023-08-01T11:59:55Z

You should be able to distinguish if you pass some option to exec handler from the run handler.

KSXGitHub · 2023-08-02T09:21:34Z

The CI always fail, but the logs doesn't tell me where.

…r-cmd-not-found

KSXGitHub · 2023-08-02T17:02:34Z

pnpm _test: ERR_PNPM_META_FETCH_FAIL GET http://localhost:7776/typescript: request to http://localhost:7776/typescript failed, reason: Socket timeout

I guess there's nothing we can do about it.

zkochan · 2023-08-02T22:30:54Z

This test fails: "pnpm exec command not found"

Locally fails too.

KSXGitHub · 2023-08-03T03:25:34Z

Oh. I think I know why I didn't found failed tests last time.

I searched for "Failed", but jest only outputs FAIL (without "ed").

KSXGitHub · 2023-08-03T05:38:39Z

@zkochan Almost all tests have passed. The ones that failed are because of timeout.

This make me think: Currently, CI tests run in a matrix of 3 node versions ✕ 2 OSes ✕ 2 events = 12 parallel jobs. I think we can skip some of them to reduce timeouts. For example: On Windows, only node 20 runs, the rest is skipped. Another alternative is serialize the node versions: node 20 is tested first, then switch to 18, then 16.

zkochan · 2023-08-03T09:32:28Z

We can serialize but it will probably make the wait longer.

exec/plugin-commands-script-runners/src/buildCommandNotFoundHint.ts

zkochan · 2023-08-03T09:57:30Z

exec/plugin-commands-script-runners/src/buildCommandNotFoundHint.ts

+
+export async function getNearestProgram (programName: string) {
+  try {
+    const binDir = path.join(process.cwd(), 'node_modules', '.bin')


This is not enough. You should check the list of commands in the current project's node_modules and in the node_modules of the workspace root.

How to get the workspace root?

This is for a hint. If the code for getting workspace root turns out to be too complicated, I think we should not implement it. After all, an incomplete but correct hint is better than an incorrect hint and a greater possibility for bugs.

workspaceDir or lockfileDir should be already passed through options.

zkochan · 2023-08-03T10:01:02Z

exec/plugin-commands-script-runners/src/exec.ts

-            err.hint = buildCommandNotFoundHint(params[0], (await readProjectManifestOnly(opts.dir)).scripts)
+            err.message = `Command "${params[0]}" not found`
+            if (opts.implicitlyFellbackFromRun) {
+              try {


why is this try needed? There is already a try/catch in getNearestProgram

Because I think readProjectManifestOnly interacts with the filesystem and therefore can fail.

Co-authored-by: Zoltan Kochan <z@kochan.io>

zkochan · 2023-08-03T12:35:30Z

exec/plugin-commands-script-runners/src/buildCommandNotFoundHint.ts

+    let programList!: string[]
+    if (workspaceDir && workspaceDir !== dir) {
+      const workspaceBinDir = path.join(workspaceDir, 'node_modules', '.bin')
+      const programListExtension = await readdir(workspaceBinDir)


I think it is OK if you just use readdir synchronously here and make the function synchronous. We don't care about the performance here so much.

performance is not critical here

zkochan · 2023-08-03T12:51:22Z

exec/plugin-commands-script-runners/src/buildCommandNotFoundHint.ts

+    const programList = readdirSync(binDir)
+    if (workspaceDir && workspaceDir !== dir) {
+      const workspaceBinDir = path.join(workspaceDir, 'node_modules', '.bin')
+      programList.push(...readdirSync(workspaceBinDir))


it is not as easy as reading all files from the .bin directory. On Windows the same command will be duplicated a few times with different extensions: .exe, .cmd, maybe even .ps1. So we need to remove the extensions.

Can the didYouMean function handle duplication, especially when we only need 1 result (the top result)?

I don't know if it can handle duplication. I would deduplicated before passing it the array

It handles duplication just fine

zkochan · 2023-08-03T14:41:34Z

There are linting errors.

exec/plugin-commands-script-runners/src/buildCommandNotFoundHint.ts

zkochan · 2023-08-04T01:16:14Z

This make me think: Currently, CI tests run in a matrix of 3 node versions ✕ 2 OSes ✕ 2 events = 12 parallel jobs. I think we can skip some of them to reduce timeouts. For example: On Windows, only node 20 runs, the rest is skipped. Another alternative is serialize the node versions: node 20 is tested first, then switch to 18, then 16.

The tests were mostly stable in the past but for the last 2-3 days the audit tests are failing very frequently. Maybe the npm registry is rate limiting requests to the audit endpoint.

technophile-04 · 2023-08-07T04:49:59Z

Tysm @KSXGitHub and @zkochan 🙌 Really appreciate this !!

related comment: #6887 (comment)

feat: friendlier error message when cmd not found

f85ef36

KSXGitHub requested a review from zkochan as a code owner August 1, 2023 10:03

zkochan reviewed Aug 1, 2023

View reviewed changes

fix: hint actual commands for exec

0a8395b

resolves #6887 (comment)

KSXGitHub requested a review from zkochan August 1, 2023 11:25

KSXGitHub added 2 commits August 1, 2023 18:26

refactor: correct function names

9c4f092

fix: lint

d0d079d

KSXGitHub added 2 commits August 2, 2023 00:53

feat: hint shouldn't crash the whole program

75df757

feat: distinguish pnpm $x from pnpm exec $x

080011f

Merge remote-tracking branch 'origin/main' into friendlier-err-msg-fo…

9faf897

…r-cmd-not-found

KSXGitHub added 3 commits August 3, 2023 10:43

test: fix

a989e62

test: add a case

bd087bc

test: add a case

092e6cf

zkochan requested changes Aug 3, 2023

View reviewed changes

KSXGitHub and others added 3 commits August 3, 2023 17:18

style: use nullish operator

653e0fd

Co-authored-by: Zoltan Kochan <z@kochan.io>

refactor: smaller try-catch block

c797936

style: no-undef-init

00028e9

KSXGitHub requested a review from zkochan August 3, 2023 10:35

feat: pass dir and workspaceDir to hint

132487f

zkochan reviewed Aug 3, 2023

View reviewed changes

refactor: simplify the code

54609d1

performance is not critical here

zkochan reviewed Aug 3, 2023

View reviewed changes

feat: remove extensions for windows

9042721

KSXGitHub commented Aug 3, 2023

View reviewed changes

exec/plugin-commands-script-runners/src/buildCommandNotFoundHint.ts Outdated Show resolved Hide resolved

KSXGitHub and others added 3 commits August 3, 2023 21:49

style: fix

6e89548

style: fix eslint

ea5a008

refactor: script runners

885c5ff

zkochan approved these changes Aug 3, 2023

View reviewed changes

zkochan merged commit c5fbdb5 into main Aug 4, 2023
13 of 14 checks passed

zkochan deleted the friendlier-err-msg-for-cmd-not-found branch August 4, 2023 00:18

technophile-04 mentioned this pull request Aug 7, 2023

Upgrade to pnpm scaffold-eth/scaffold-eth-2#444

Closed

KSXGitHub added a commit that referenced this pull request Aug 7, 2023

ci: serialize the jobs

b3cda69

related comment: #6887 (comment)

KSXGitHub mentioned this pull request Aug 7, 2023

ci: serialize the jobs #6917

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: friendlier error message when command not found #6887

feat: friendlier error message when command not found #6887

KSXGitHub commented Aug 1, 2023 •

edited

zkochan Aug 1, 2023

KSXGitHub commented Aug 1, 2023

zkochan commented Aug 1, 2023

KSXGitHub commented Aug 2, 2023

KSXGitHub commented Aug 2, 2023

zkochan commented Aug 2, 2023

KSXGitHub commented Aug 3, 2023

KSXGitHub commented Aug 3, 2023

zkochan commented Aug 3, 2023

zkochan Aug 3, 2023

KSXGitHub Aug 3, 2023

zkochan Aug 3, 2023

zkochan Aug 3, 2023

KSXGitHub Aug 3, 2023

zkochan Aug 3, 2023

zkochan Aug 3, 2023 •

edited

KSXGitHub Aug 3, 2023

zkochan Aug 3, 2023

KSXGitHub Aug 3, 2023

zkochan commented Aug 3, 2023

zkochan commented Aug 4, 2023

technophile-04 commented Aug 7, 2023

feat: friendlier error message when command not found #6887

feat: friendlier error message when command not found #6887

Conversation

KSXGitHub commented Aug 1, 2023 • edited

Choose a reason for hiding this comment

KSXGitHub commented Aug 1, 2023

zkochan commented Aug 1, 2023

KSXGitHub commented Aug 2, 2023

KSXGitHub commented Aug 2, 2023

zkochan commented Aug 2, 2023

KSXGitHub commented Aug 3, 2023

KSXGitHub commented Aug 3, 2023

zkochan commented Aug 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zkochan Aug 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zkochan commented Aug 3, 2023

zkochan commented Aug 4, 2023

technophile-04 commented Aug 7, 2023

KSXGitHub commented Aug 1, 2023 •

edited

zkochan Aug 3, 2023 •

edited