-
Notifications
You must be signed in to change notification settings - Fork 450
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
feat(cli): show prompt if local version doesn't match remote #6707
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
No changes to documentation |
Component Testing Report Updated Jun 11, 2024 9:17 PM (UTC)
|
// Get the version without any tags if any | ||
const coercedSanityVersion = semver.coerce(installedSanityVersion)?.version | ||
if (autoUpdatesEnabled && !coercedSanityVersion) { | ||
throw new Error(`Failed to parse installed Sanity version: ${installedSanityVersion}`) | ||
} | ||
const version = encodeURIComponent(`^${coercedSanityVersion}`) | ||
const autoUpdatesImports = getAutoUpdateImportMap(version) |
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.
Can we move this into the conditional below? Feels like unnecessary work and potentially throwing if the user has not opted in to auto updates
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.
@rexxars the reason I have it outside the conditional is because the autoUpdatesImports
variable is used below in line 160
If I make a variable like let autoUpdatesImports
TS can't necessarily tell if the variable is initialized so it complains. The problem with moving the whole thing after the clean is that it does into a spinner and prompt doesn't work with spinner. I think there is some opportunity to clean this up a little bit but unless you strongly feel that we should do it now, punt it for future work?
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.
Makes sense. Can refactor later 👍
Description
Shows a prompt if the installed version of importmap deps are different then what it would resolve to.
FIXES SDX-1342
What to review
Updates makes sense.
To test no error case in test-studio
dev/test-studio/
pnpm sanity:build --auto-updates
Expected: No warning
To test error case in test-studio
sanity
and/or@sanity/vision
version to something older indev/test-studio/package.json
and runpnpm install
dev/test-studio
pnpm sanity:build --auto-updates
Expect: will show copy with different versions
Testing
It's probably very hard to do a more integration level test for this, I wrote a unit test for the compare function at least
Notes for release
N/A