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: add a new message telling the user that a new version of Astro is available #10734
Conversation
🦋 Changeset detectedLatest commit: ad7d5ca The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
This PR is blocked because it contains a minor
changeset. A reviewer will merge this at the next release if approved.
Can you add screenshots of both? |
Hmm personally I'm not a fan of tools that yap about new versions (turbo, pnpm) as I'd upgrade dependencies myself when I have a reason to, but maybe that's just me 😅 (dev toolbar only is fine though) |
I can agree! Though I think the message here isn't quite as visually bold as pnpm's or turbo's, so it doesn't bother me as much personally. I also cannot deny that |
Core discussed async and this is the current status:
|
@@ -161,3 +162,56 @@ async function installPackage( | |||
return false; | |||
} | |||
} | |||
|
|||
export async function fetchPackageJson( |
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.
Fished this out of astro add
, code is unchanged. astro add
is insanely big so fishing things out of it is honestly just good.
if (res.status >= 200 && res.status < 300) { | ||
return await res.json(); | ||
} else if (res.status === 404) { | ||
// 404 means the package doesn't exist, so we don't need an error message here |
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.
This is a quite odd pattern that astro add
does everywhere where functions return errors, kinda neat
} | ||
|
||
if (preferences) { | ||
await preferences.set('_variables.lastUpdateCheck', Date.now(), { reloadServer: false }); |
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.
So, I had a choice to make here:
- Use a new file, a new system, etc to store variables like this
- Re-use preferences under a private namespace
I went with the later when I realized adding a new store with different values to preferences would require change a lot of type utils and stuff for very little value. I think it's not a problem... You're not supposed to change the config file manually, and the CLI won't show this private namespace.
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.
That definitely seems reasonable. We have this same problem with telemetry, too, so probably worth generalizing a pattern at somepoint.
Quick comment from my side, would the update checker fail for users who don't have |
Description was outdated, doesn't use |
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.
Look great, awesome work! Just a few small comments, but nothing blocking!
// A copy of this function also exists in the create-astro package | ||
let _registry: string; |
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 this default to process.env.npm_config_registry
or is that value just totally irrelevant?
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.
The value seems to be irrelevant in many cases, I've even seen npm issues saying that it doesn't respect it. No idea how that works.
|
||
export async function shouldCheckForUpdates(preferences: AstroPreferences): Promise<boolean> { | ||
const timeSinceLastCheck = Date.now() - (await preferences.get('checkUpdates._lastCheck')); | ||
const hasCheckUpdatesEnabled = await preferences.get('checkUpdates.enabled'); |
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.
That's very nice! Not sure if we have a preferences reference page yet, but this would definitely be something to document.
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.
Approving for docs!
Changes
Check for update on startup of the dev command. This will appear in both the console and the dev toolbar's home app and can be disabled if unwanted.
There's a few heuristics to make it less annoying:
We can make this more or less strict, it's two constants.
Testing
Tested manually
Docs
There's some docs in there!