-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update sfdx-lwc-jest bin name #17
Conversation
messages/setup.json
Outdated
@@ -6,7 +6,6 @@ | |||
"errorNpmNotFound": "npm command not found. Verify npm is properly installed and try again.", | |||
"errorNodeVersion": "Node.js version too low. Version 8.12.0 or higher required. Found version %s", | |||
"errorNoPackageJson": "No package.json found at root of project. Run \"npm init\" to create one.", | |||
"errorLwcJestInstall": "Error installing @salesforce/sfdx-lwc-jest: %o", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this error message not required anymore?
const yarnLockExists = fs.existsSync(path.join(this.project.getPath(), 'yarn.lock')); | ||
if (yarnLockExists) { | ||
this.ux.log('Detected yarn.lock file, using yarn commands'); | ||
lwcJestInstallRet = execSync('yarn add --dev @salesforce/sfdx-lwc-jest', { stdio: 'inherit' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you removing this because the execution is never expected to fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before we called spawnSync
which would return an error prop that we could check and output a pretty error with. This execSync
call passes through the stdio output to the user so they'll see any success or errors messages from the npm call.
The call also throws if there's an error, which I think is fine. We could catch and rethrow as a SfdxError
like we used to, but. I don't think the error message would provide any additional value...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is any special handling for SfdcError objects, then it would be worth wrapping it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I guess we can keep the error type just in case and to stay consistent with previous behavior. Added it back.
The
lwc-jest
executable has been deprecated for a while. We should always be referencingsfdx-lwc-jest
.@salesforce/sfdx-lwc-jest
actually references both executable names in thebin
field of its package.json so referencing the deprecated name still works on Mac. On Windows, however, we have to reference the executable that's nested inside the package (node_modules/sfdx-lwc-jest/bin/lwc-jest
) which does not exist.Also cleaning up how we handle the return of the
execSync
call since we recently converted that call fromspawnSync
and they return differently.Related Stack Exchange post: https://salesforce.stackexchange.com/questions/308672/sfdx-forcelightninglwctestrun-not-working