Skip to content
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

Merged
merged 2 commits into from
Feb 10, 2023
Merged

Conversation

henrinormak
Copy link
Contributor

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.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 8, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: henrinormak / name: Henri Normak (870438a)

@henrinormak
Copy link
Contributor Author

@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)

@henrinormak henrinormak changed the title fix: expose name on Dataloader instance fix: expose name in Dataloader instance types Feb 8, 2023
*
* Is `null` if not set in the constructor.
*/
name: string | null;
Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Member

@saihaj saihaj left a 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

@henrinormak
Copy link
Contributor Author

Added the changeset

@saihaj saihaj merged commit e286f66 into graphql:main Feb 10, 2023
@saihaj
Copy link
Member

saihaj commented Feb 13, 2023

https://github.com/graphql/dataloader/releases/tag/v2.2.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants