-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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!: improve collection / global slugs type-safety in various places #8311
base: beta
Are you sure you want to change the base?
fix!: improve collection / global slugs type-safety in various places #8311
Conversation
…e-type-safety-of-collection-and-global-slugs
I'd like @denolfe to review this also before we merge it in and we need to have a |
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 think this is a good change that will help DX. Left a few questions.
@@ -79,7 +80,7 @@ export interface CollectionOptions { | |||
} | |||
|
|||
export interface PluginOptions { | |||
collections: Record<string, CollectionOptions> |
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.
What is the reasoning for this to be a partial?
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.
@@ -26,7 +26,7 @@ export default buildConfig({ | |||
plugins: [ | |||
vercelBlobStorage({ | |||
collections: { | |||
[Media.slug]: true, | |||
media: true, |
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.
Does TS still allow using Media.slug
without error? Having people change all their slugs to as const
seems less than ideal.
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.
No, because Media.slug
itself has a type of string
, not 'media'
. Other way around it is to write collection like this:
It would work if the slug
property itself of CollectionConfig
would be of the CollectionSlug
type, though.
But the drawback of this is that you wouldn't be able to define a new collection slug without typescript error, which can be fixed only after types generation. That's why I only did it for SanitizedCollectionConfig['slug']
…e-type-safety-of-collection-and-global-slugs
BREAKING:
Improves type-safety of collection / global slugs by using
CollectionSlug
andGlobalSlug
types instead ofstring
in these places:payload.collections
payload.db
collections
argument of plugins: storage-adapters, nested-docs, cloudproperty of
ListDrawerProps`collectionSlugs
andselectCollectionSlugs
arguments ofuseListDrawer
Permissions
ofAuthResult
This also changes how we suggest to add a upload collection to cloud-storage adapter:
Before:
After:
It's not needed anymore, as keys of the
collection
argument are type-safe now andMedia.slug
from this code actually hasstring
type so this will lead to a TS error.If you still want to use
Media.slug
, you can use this notation:This way,
Media.slug
is typed as'media'
instead ofstring
and can be used asCollectionSlug
.