onSuccess & Watch Fix

[methompson]

fixes #1245

Removed an await line for tinyexec-based binary executation command.
Watch commands would not trigger if an onSuccess command was provided.

[codesandbox]

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders

Open Preview

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
tsup	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 2, 2024 4:02pm

[laat]

-                  await onSuccessProcess
-                  if (
-                    onSuccessProcess.exitCode &&
-                    onSuccessProcess.exitCode !== 0
-                  ) {
-                    process.exitCode = onSuccessProcess.exitCode
-                  }
+                  onSuccessProcess.process?.on('exit', (code) => {
+                    if (code && code !== 0) {
+                      process.exitCode = code
+                    }
+                  })

In stead of only removing the await we should probably reflect the behaviour it had before it broke.

[methompson]

-                  await onSuccessProcess
-                  if (
-                    onSuccessProcess.exitCode &&
-                    onSuccessProcess.exitCode !== 0
-                  ) {
-                    process.exitCode = onSuccessProcess.exitCode
-                  }
+                  onSuccessProcess.process?.on('exit', (code) => {
+                    if (code && code !== 0) {
+                      process.exitCode = code
+                    }
+                  })

In stead of only removing the await we should probably reflect the behaviour it had before it broke.

I'd agree: unfortunately, it appears the previous commit wasn't quite as interested in doing the same: 4dd5bfe#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80

Note that the await statement was added. By removing it, we're getting closer to the original intention. Unfortunately, tinyexec does not appear to be using standard promises. When I attempted to add .catch to onSuccessProcess, I was confronted with an error indicating that it's not a function.

If we're to go back to previous functionality, we should probably move back to execa (or just use Node's built-in exec)

[laat]

when we do not await we will not capture the exit code ever. The .on('exit', ...) listener will capture the exit code as it was originally done with execa.

[methompson]

when we do not await we will not capture the exit code ever. The .on('exit', ...) listener will capture the exit code as it was originally done with execa.

And when we do await, we completely break use of onSuccess. I think removing the await line is a good compromise of allowing onSuccess to function correctly before someone else figures out why tinyexec was used in the first place and either excising it in lieu of a better exec function (like Node's built in ChildProcess).

[methompson]

@laat I've revised the code in this PR to now show errors on save, while also retaining the onSuccess function and watch function.

I tested this in another project of mine where 8.3.5 did not function correctly. Watching/onSuccess worked as intended and errors were shown in the CLI.

[laat]

We should probably not produce promises that we do not catch or await (by way of a async processExitHandler), as this will very often lead to unhandledRejection errors resulting in the watch process exiting. It's unlikely in this scenario, but still a code smell to be avoided.

This is the proper fix #1256

[methompson]

We should probably not produce promises that we do not catch or await (by way of a async processExitHandler), as this will very often lead to unhandledRejection errors resulting in the watch process exiting. It's unlikely in this scenario, but still a code smell to be avoided.

This is the proper fix #1256

The problem is that processExitHandler doesn't actually throw. It's a PromiseLike and has no catch function, only then.

[laat]

the processExitHandler result is a real promise. tinyexec is promiselike

[methompson]

the processExitHandler result is a real promise. tinyexec is promiselike

if onSuccessProcess never throws, processExitHandler won't throw.

[laat]

if onSuccessProcess never throws, processExitHandler won't throw.

I've addressed this, read my comment again.