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

feat(types): expose GraphQlQueryResponseData type #256

Merged
merged 4 commits into from
Feb 2, 2021

Conversation

oscard0m
Copy link
Member

@oscard0m oscard0m commented Feb 2, 2021

πŸ“ Summary

Expose GraphQlQueryResponseData type

β›± Motivation and Context

Fix #254

src/index.ts Outdated
@@ -13,6 +13,8 @@ export const graphql = withDefaults(request, {
url: "/graphql",
});

export { GraphQlQueryResponseData } from './types'
Copy link
Member Author

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)

@wolfy1339 @G-Rath

Copy link
Member

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)

Copy link
Member Author

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

Copy link
Contributor

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?

https://github.com/octokit/rest.js/blob/fc62e6b98625830e1c60ba6c630306468b4c65a9/package.json#L66

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oscard0m oscard0m added Type: Feature New feature or request typescript Relevant to TypeScript users only labels Feb 2, 2021
@gr2m gr2m merged commit f697627 into master Feb 2, 2021
@gr2m gr2m deleted the expose-GraphQlQueryResponseData-type branch February 2, 2021 21:46
@github-actions
Copy link

github-actions bot commented Feb 2, 2021

πŸŽ‰ This PR is included in version 4.6.0 πŸŽ‰

The release is available on:

Your semantic-release bot πŸ“¦πŸš€

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature or request typescript Relevant to TypeScript users only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GraphQlQueryResponseData type not exposed when package is published
3 participants