-
Notifications
You must be signed in to change notification settings - Fork 393
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: added context with client and schema to slugify and source options #3711
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
) => string | Promise<string> | ||
|
||
export type SlugifierFn = (source: string, schemaType: SlugSchemaType) => string | Promise<string> | ||
|
||
// TODO: De-dupe with validation types |
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.
TOdone
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.
Nice! Noted a couple of non-required suggestions for improvement.
@@ -9,12 +9,16 @@ const defaultSlugify = (value: FIXME, type: SlugSchemaType): string => { | |||
return value ? speakingurl(value, slugifyOpts) : '' | |||
} | |||
|
|||
export function slugify(sourceValue: FIXME, type: SlugSchemaType): Promise<string> { | |||
export function slugify( |
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.
suggestion: same as above re: converting async function. I think we want the error to propagate on the returned promise (I do realise that this isn't "new" behavior, so feel free to ignore)
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.
(and by "above" I mean "below" 🙃)
return {parentPath, parent, ...context} | ||
} | ||
|
||
function getNewFromSource( |
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.
suggestion: make this an async function? That way we make sure that any (current or future) sync errors triggered by either this function or the slugifier call are thrown on the returned promise. As a bonus, the Promise.resolve call in here can be removed too.
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.
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.
Seems like we should turn off the require-await
rule or replace it with @typescript/require-await
which uses type information: https://typescript-eslint.io/rules/require-await/
Description
Extends the context object passed to
options.source
andoptions.slugify
with getClient, schema, currentUser (and projectId and dataset for consistency with initial value context).Also fixes the context type for
options.isUnique
which was duplicated in the definition, and missing some properties (they where available at runtime already though, via validation context).This is primarily to make it possible to use the client when using these options; a common thing in v2.
What to review
All the code.
Notes for release
Slug options
source
andslugify
are now passed acontext
parameter as the last argument. It containsgetClient
,schema
andcurrentUser
.