-
Notifications
You must be signed in to change notification settings - Fork 506
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: expose name
in Dataloader instance types
#334
Conversation
|
@saihaj perhaps you can help out with this one? Makes the new feature impossible to use in TS (as if you only have access to the dataloader instance you cannot safely read the name) |
name
in Dataloader instance types
* | ||
* Is `null` if not set in the constructor. | ||
*/ | ||
name: string | null; |
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 can also be undefined
given in config it is name?: string | null
if you don't give a name
then that is 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.
Actually, the constructor then defaults to null - https://github.com/graphql/dataloader/blob/main/src/index.js#L478-L484
The config also specifically mentions this defaulting - https://github.com/graphql/dataloader/blob/main/src/index.d.ts#L124
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.
also can you run yarn changeset
and create a patch
changeset explaining the change
Added the changeset |
Followup to #331 and #326
This corrects the types, as the name property is also exposed on the Dataloader instance, not only in the config. Tests already test for this, but as they are not in TS, the types were not checked.