Refactor CLI command runner #708
Conversation
aswamy
force-pushed
the
refactor-cli-command-runner
branch
from
412a06c to
18d4a09
Compare
| @@ -0,0 +1,5 @@ | |||
| --- | |||
There was a problem hiding this comment.
Curious if you need a changeset for a functionality neutral refactor
There was a problem hiding this comment.
I think changesets with [internal] get removed. But you are correct we don't need it here.
There was a problem hiding this comment.
+1 we can prob do without a changeset for internal changes
There was a problem hiding this comment.
No. You do need a changeset because if vscode-extension depends on this and it doesn't get a bump, then you'll have in-prod-only errors. We should always have a changeset for changes to exports. Even if only used internally.
There was a problem hiding this comment.
It's less relevant for vscode-extension, but I want to make this clear:
Think of this situation:
theme-checkis version1.0.0theme-language-serveris version2.0.0theme-checkhas a new export but doesn't get a changesettheme-language-serveruses that export, creates a changeset- next time we publish
- theme-check-common stays at
1.0.0. 1.0.0 does NOT have the new export! - theme-language-server becomes
2.1.0, depends on1.0.0
- theme-check-common stays at
- Works fine locally because you use code from the folder
- Folks that download theme-language-server from npm get
2.1.0and1.0.0 - 1.0.0 doesn't have the export
- Uh oh! Breaks in prod.
There was a problem hiding this comment.
Thanks for the info @charlespwd!
| const executables = stdout | ||
| .replace(/\r/g, '') | ||
| .split('\n') | ||
| .filter((exe) => exe.endsWith('bat')); |
There was a problem hiding this comment.
For my own learning: When do we encounter a bat? How did you you test this on windows / know what cases to cover?
There was a problem hiding this comment.
.bat is the .sh of Windows. Most scripts are shipped as .bat on windows and IIRC the where.exe thing will give you a couple of results and you usually want the .bat one.
| const shopifyCliPathPromise = getShopifyCliPath(); | ||
|
|
||
| async function getShopifyCliPath() { | ||
| if (isWin) { |
|
Should we cut a ticket to add error handling in the future? Maybe a notification in the client? This happens if not auth'ed into CLI. |
What are you adding in this PR?
Follows up comment by @madmath #597 (comment)
Refactor our
metafields pullcommand into a proper CLI command runnerBefore you deploy
changeset