-
Notifications
You must be signed in to change notification settings - Fork 85
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
feat(types): expose GraphQlQueryResponseData type #256
Conversation
src/index.ts
Outdated
@@ -13,6 +13,8 @@ export const graphql = withDefaults(request, { | |||
url: "/graphql", | |||
}); | |||
|
|||
export { GraphQlQueryResponseData } from './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.
Is there a π and easy way to test this type is being exposed properly at Continuous Integration level?
This means, when somebody installing latest version of octokit/grapqhql.js
will be able to write import { GraphQlQueryResponseData } from '@octokit/graphql.js'
(I know I can manually test it with pika's package but I was wondering if there's a way to integrate it at CI level)
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.
test this type is being exposed properly
The fact that you're doing export x from y
means that it should be exported for use; if it isn't, then that represents a far bigger issue in TypeScript itself :)
The way to test this on our end would be to have a file doing the import
- I'm not against this in a few places (like how we've got a typescript validation file in webhooks.js
), but want to make sure we don't end up in a mindset that is us not trusting typescript, as that'll mean we have to write an import for everything we export which'll be really painful (since we'll effectively have to double the amount of coding we do, and defeat a huge part of the reason for using TypeScript)
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 see your point... I guess maintaining documentation would be enough in this case if we decide to remove at some point the exposure of something :)
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.
Good point! Maybe we could do a similar typescript verification test as we do in @octokit/rest
?
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 PR is included in version 4.6.0 π The release is available on: Your semantic-release bot π¦π |
π Summary
Expose GraphQlQueryResponseData type
β± Motivation and Context
Fix #254