-
Notifications
You must be signed in to change notification settings - Fork 129
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
Fix issue with CLI2 process printing tasks inside concurrent output #1194
Conversation
This comment has been minimized.
This comment has been minimized.
c74fd70
to
bd3f1bb
Compare
Benchmark reportThe following table contains a summary of the startup time for all commands.
|
Coverage report
Show new covered files 🐣
Test suite run success949 tests passing in 484 suites. Report generated by 🧪jest coverage report action from f7744d9 |
if (exists) { | ||
await tasks[0]!.task() | ||
if (!exists) stdout.write('Installing theme dependencies...') | ||
const usingLocalCLI2 = Boolean(process.env.SHOPIFY_CLI_2_0_DIRECTORY) |
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.
I'd adjust this one to use the isTruthy
function for this since the environment variable can take values that can't be coerced to a boolean.
if (exists) { | ||
await tasks[0]!.task() | ||
if (!exists) stdout.write('Installing theme dependencies...') | ||
const usingLocalCLI2 = isTruthy(process.env.SHOPIFY_CLI_2_0_DIRECTORY) |
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.
isTruthy
return
s ['1', 'true', 'TRUE', 'yes', 'YES'].includes(variable)
but SHOPIFY_CLI_2_0_DIRECTORY
is e.g."/Users/poitrin/src/github.com/Shopify/shopify-cli"
Therefore, this can't work :-(
Boolean('x')
returns true
, which is okay.
WHY are these changes introduced?
I've extracted this fix from #1185
WHAT is this pull request doing?
Fix execCLI2 running tasks inside renderConcurrent. Previously renderTasks was trying to be rendered inside renderConcurrent, but that's not possible.
How to test your changes?
pnpm shopify app dev --path fixtures/app
Measuring impact
How do we know this change was effective? Please choose one:
Checklist
dev
ordeploy
have been reflected in the internal flowchart.