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(gatsby-graphiql-explorer): add support for magic fragments #28878

Merged
merged 5 commits into from
Jan 14, 2021

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Jan 5, 2021

Description

This is initial integration for upcoming graphiql feature where we can register fragments and they will be available in graphiql ( graphql/graphiql#1750 ). This feature was recently released, so we are able to bump it soon.

All the kudos go to @acao for the work they already did on adding support for it and helping hand when working on adding support for it in gatsby

And here's sneak peek:
Screenshot 2021-01-05 at 22 33 44

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jan 5, 2021
@@ -193,6 +193,15 @@ module.exports = {
} else {
graphiqlExplorer(app, {
graphqlEndpoint,
getFragments: function getFragments(): Array<FragmentDefinitionNode> {
Copy link
Contributor

Choose a reason for hiding this comment

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

😎

@pieh pieh removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jan 5, 2021
@acao
Copy link
Contributor

acao commented Jan 11, 2021

awesome work! @pieh i was going to offer to keep going with this since I’ve been cleared to keep improving gatsby graphiql as I wrap up here, would that be helpful? seems like this PR is almost finished anyways!

@pieh
Copy link
Contributor Author

pieh commented Jan 11, 2021

awesome work! @pieh i was going to offer to keep going with this since I’ve been cleared to keep improving gatsby graphiql as I wrap up here, would that be helpful? seems like this PR is almost finished anyways!

I just pushed what I think is required change to get this in (bump for graphiql and optional for graphiql-code-exporter + explict runtime-regenerator` which was removed in graphiql at some point, but we kind of need it), not sure if we want to tackle some form of hot updates for fragments.

I checked and graphiql in IE11 currently doesn't work anyway, so I'm not breaking it anymore than it already is

@acao
Copy link
Contributor

acao commented Jan 11, 2021

yes, we dropped IE support in late 2019. it may be that codemirror has as well.

hot updates should work using subscriptions if we supply the prop, but we may need some lifecycle tweaks

@pieh
Copy link
Contributor Author

pieh commented Jan 11, 2021

One thing to consider is adding test case for magic fragments in https://github.com/gatsbyjs/gatsby/blob/master/e2e-tests/development-runtime/cypress/integration/functionality/graphql-endpoint.js - not sure if we can easily test if hint and lint work, but at least execution part would be quite doable

@acao
Copy link
Contributor

acao commented Jan 11, 2021

yes we just need to test whether the prop gets passed. we already have plenty of tests in place for these new validation and completion features related to this

@pieh
Copy link
Contributor Author

pieh commented Jan 11, 2021

hot updates should work using subscriptions

Our graphql server doesn't have subscriptions, so it would need some setup to go through it, but we can likely do something that need less lift to let our graphiql app know that it needs to refetch fragments (that's not the most performant thing ... but also not sure if it makes much sense to optimise for fragment hot reloading when it comes to time effort needed to pull it off)

And lastly - I'm not sure if we need to support hot fragment updates - at least for this PR - I just wanted to mention that it's not supported to not cause confussion (be explicit about what is supported with this PR)

@acao
Copy link
Contributor

acao commented Jan 11, 2021

oh yes of course. I would expect by the end of this PR, users would need to refresh to see updated plugin fragments after, say, adding or removing a plugin, or defining your own.

acao
acao previously approved these changes Jan 11, 2021
Copy link
Contributor

@acao acao left a comment

Choose a reason for hiding this comment

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

tested locally and thoroughly, and everything looks great to ship here! It all appears to work just fine as expected. Just reviewing this PR gave me a few takeaways to improve GraphiQL and Explorer!

only thing is maybe mention it in the gatsby-graphiql-explorer readme? oh and I should notify docs team as well!

@pieh pieh marked this pull request as ready for review January 12, 2021 16:49
Copy link
Contributor

@acao acao left a comment

Choose a reason for hiding this comment

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

all tested, looks good to go!

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.

2 participants