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

Bring back getDescription export #1165

Merged
merged 1 commit into from
Dec 19, 2017
Merged

Bring back getDescription export #1165

merged 1 commit into from
Dec 19, 2017

Conversation

stubailo
Copy link

We were using this in graphql-tools with graphql@0.11, would be nice to get it exported again!

@stubailo
Copy link
Author

OK looks like there are some typing things to work through, I will come back to it tomorrow.

@leebyron
Copy link
Contributor

You should also have this exported from index.js if you expect to use it in a dependent library, otherwise it might be too easy to regress again! We assume exports used between files are used purely internally.

It might be helpful to audit for other deep require uses as well to avoid future breakage

@stubailo
Copy link
Author

@leebyron happy to make that modification for sure, we definitely have 3-4 others we are using. Didn't realize that was the intention with the internal exports!

Any idea what the argument type should be here? I would have thought ASTNode would have been the right thing, but I guess not all nodes have descriptions. Maybe I should just write some custom duck type thing.

@leebyron
Copy link
Contributor

Try out { +description?: StringValueNode }?

We were using this in graphql-tools with graphql@0.11, would be nice to get it exported again!

Update buildASTSchema.js

Fix formatting

Add types for getDescription

Export getDescription from the root
@stubailo
Copy link
Author

OK looks like it's all good now, thanks for the suggestions!

@leebyron leebyron merged commit 8173c24 into graphql:master Dec 19, 2017
@leebyron
Copy link
Contributor

Nice. I'll likely have a point release out today or tomorrow that will include this.

Also, apparently this is your first commit to GraphQL.js. Congrats! 🎉

@stubailo
Copy link
Author

Also, apparently this is your first commit to GraphQL.js. Congrats!

That is SO SURPRISING. I'll look into contributing more! I guess I spent more time on GraphQL.org that I misattributed.

@nodkz
Copy link
Contributor

nodkz commented Dec 23, 2017

I'm also using getDescription method in graphql-compose.
Glad that it already fixed. Thanks, Sashko! 👍

@negezor
Copy link

negezor commented Jan 18, 2018

@leebyron hi, when can you update package?

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

Successfully merging this pull request may close these issues.

5 participants