Skip to content

TS: introspectionTypes should be array of GraphQLNamedType #2261

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 17, 2019
Merged

TS: introspectionTypes should be array of GraphQLNamedType #2261

merged 2 commits into from
Nov 17, 2019

Conversation

pcarrier
Copy link
Contributor

@pcarrier pcarrier commented Nov 15, 2019 •
edited
Loading

Addresses #2260

glasser
glasser reviewed Nov 15, 2019
View reviewed changes
src/type/introspection.d.ts Outdated
@@ -35,6 +35,6 @@ export const SchemaMetaFieldDef: GraphQLField<any, any>;
export const TypeMetaFieldDef: GraphQLField<any, any>;
export const TypeNameMetaFieldDef: GraphQLField<any, any>;

export const introspectionTypes: ReadonlyArray<GraphQLType>;
export const introspectionTypes: ReadonlyArray<GraphQLNamedType>;

export function isIntrospectionType(type: GraphQLType): boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change this to take GraphQLNamedType too (which is how the actual function is declared in Flow)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it, but looks to me like it'd work with any GraphQLType and making the API more strict would break backward compatibility?

Copy link
Contributor

@glasser glasser Nov 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm? The implementation looks at the name field of its argument, making the TS type (but not the Flow type) just wrong.

@IvanGoncharov IvanGoncharov changed the title #2260: introspectionTypes are named TS: introspectionTypes should be array of GraphQLNamedType Nov 17, 2019
@IvanGoncharov IvanGoncharov changed the title TS: introspectionTypes should be array of GraphQLNamedType TS: introspectionTypes should be array of GraphQLNamedType Nov 17, 2019
@IvanGoncharov IvanGoncharov added the PR: bug fix 🐞 requires increase of "patch" version number label Nov 17, 2019
@IvanGoncharov IvanGoncharov merged commit 70ec03d into graphql:master Nov 17, 2019
@IvanGoncharov
Copy link
Member

@glasser Thanks for review 👍

@pcarrier Thanks for PR 👍
I addressed @glasser suggesting and fixed build (you forgot to import GraphQLNamedType).
In general Flow and TS, typings should be as close as possible.

So now it's merged and will be included in 15.0.0-alpha.2.
Do you need this to be backported into 14.x.x?

@glasser
Copy link
Contributor

glasser commented Nov 18, 2019

We wouldn't complain about a backport but can certainly survive with an as if not.

@IvanGoncharov
Copy link
Member

@glasser Thanks 👍 Will try to push 15.0.0 ASAP.

@pcarrier pcarrier deleted the pcarrier/introspection.d.ts branch November 19, 2019 03:16
@IvanGoncharov IvanGoncharov mentioned this pull request Nov 26, 2019
Closed
@snyk-bot snyk-bot mentioned this pull request Feb 10, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glasser glasser glasser left review comments

Assignees
No one assigned
Labels
PR: bug fix 🐞 requires increase of "patch" version number
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pcarrier @IvanGoncharov @glasser