Conversation
AugustinMauroy
added
Fast track 🤘
awaiting reviewer
| @@ -1,4 +1,4 @@ | |||
| import { execSync } from 'node:child_process'; | |||
| import { spawn } from 'node:child_process'; | |||
There was a problem hiding this comment.
Would it be worth adding some unit tests with JSSG? (both to confirm the execSync error and the expected spawn behaviour`?)
There was a problem hiding this comment.
in next pr, because now it's just a patch. also codemod team propose to use json ast on package.json to remove dep & then run install command. I'm going to explore that.
There was a problem hiding this comment.
Why not in this PR to ensure the patch works? The lack of a test is what caused it to slip through in the first place.
There was a problem hiding this comment.
@mohebifar how can we allow capability on jssg test ?
There was a problem hiding this comment.
npx codemod jssg test --allow-child-process ...
JakobJingleheimer
added
awaiting author
JakobJingleheimer
left a comment
JakobJingleheimer
left a comment
There was a problem hiding this comment.
This looks good, but please add a test to confirm the fix is working (and prevent it happening again).
5 participants
execSyncdidn't exist on LLRT