-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Support deprecating Web Vitals table #11096
Conversation
🦋 Changeset detectedLatest commit: dedabbc 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.
I wish npm would allow some preunitinstall
scripts, so libraries can clean after themselves. I suppose that's the best we can do for now.
@@ -2,7 +2,8 @@ import { defineDbIntegration } from '@astrojs/db/utils'; | |||
import { AstroError } from 'astro/errors'; | |||
import { WEB_VITALS_ENDPOINT_PATH } from './constants.js'; | |||
|
|||
export default function webVitals() { | |||
export default function webVitals({ deprecated }: { deprecated?: boolean } = {}) { | |||
process.env.DEPRECATE_WEB_VITALS = deprecated ? 'true' : undefined; |
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.
Shouldn't we set this only when running a db push
? Or is it intentional?
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 guess you might want it in dev
etc. too? If you happened to run it while deprecated. Probably doesn’t matter either way though — I just wanted it to match how a user-defined table might behave really.
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.
README and changeset LGTM, Chris!
Changes
deprecated: true
on a table and push that before it will allow you to remove it completely. Because integrations control the tables themselves this is not something users can do themselves, so this PR reflects that up to the integration options.Testing
Tested manually that the
deprecated
property is being set properly on the table config.Docs
/cc @withastro/maintainers-docs for feedback on the README changes!