percy / cli

The Percy CLI is used to interact with, and upload snapshots to, percy.io via the command line.
https://docs.percy.io/docs/cli-overview
70 stars 43 forks source link

Using PERCY_ENABLE can lead to NPE #1560

Closed SemaLuna closed 5 months ago

SemaLuna commented 6 months ago

The problem

When using PERCY_ENABLE to disable Percy, the exec command unfortunately still has logic for trying to send buildEvents upstream (when there is an error).

So if the command that percy wraps on errors, we get a NPE.

Eventually leading to an NPE:

file:///PATH_TO_PROJECT/node_modules/.pnpm/@percy+cli-exec@1.28.0_typescript@5.4.2/node_modules/@percy/cli-exec/dist/exec.js:145
          percy.client.sendBuildEvents(percy.build.id, myObject);
                ^
TypeError: Cannot read properties of undefined (reading 'client')
    at ChildProcess.<anonymous> (file:///PATH_TO_PROJECT/node_modules/.pnpm/@percy+cli-exec@1.28.0_typescript@5.4.2/node_modules/@percy/cli-exec/dist/exec.js:145:17)
    at ChildProcess.emit (node:events:513:28)
    at maybeClose (node:internal/child_process:1100:16)
    at Socket.<anonymous> (node:internal/child_process:458:11)
    at Socket.emit (node:events:513:28)
    at Pipe.<anonymous> (node:net:301:12)

Environment

Code to reproduce issue

It is easy enough to notice the issue in the code - in this line we try to sendBuildEvents, however percy is undefined when PERCY_ENABLE is 0 (you can see this assumption in the same file here)

Should be a simple fix.

prklm10 commented 6 months ago

Hi @SemaLuna, Sorry for the late revert we will fix look into this and the fix will be available in the next stable release.

SemaLuna commented 5 months ago

@prklm10 no problem at all - thanks for the fix.

prklm10 commented 5 months ago

Hi @SemaLuna we have fixed this bug in 1.28.3.

prklm10 commented 5 months ago

Closing this issue.