Skip to content

Update GraphiQL #1069

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

Merged
merged 3 commits into from
Aug 28, 2023
Merged

Update GraphiQL #1069

merged 3 commits into from
Aug 28, 2023

Conversation

Empty2k12
Copy link
Contributor

GraphiQL has not been updated in ages (used version is 0.17.5, current version is 1.9.3). Since then, a lot of nice features such as the Request Headers Editor and XSS-Fixes have landed.

This PR updates GraphiQL according to the graphiql-cdn example with the addition of passing request headers and keeping the subscription logic.

@tyranron tyranron added enhancement Improvement of existing features or bugfix k::integration Related to integration with third-party libraries or systems labels May 26, 2022
@tyranron
Copy link
Member

@Empty2k12 thanks for bringing this up!

GraphiQL and GraphQL Playground updates go fully unnoticed for us. I think it's time do to something about it.

It would be nice to add a package.json manifest to the repository with a versions range of GraphiQL and GraphQL Playground (along with the yarn.lock file), where in the post-install stage we copy the sources of GraphiQL/GraphQL Playground to juniper/src/ dir, and in the code we use include_str!() at the propriate places.

Finally, we make dependabot to look after GraphiQL/GraphQL Playground updates for us.

What do you think about this?

@Empty2k12
Copy link
Contributor Author

@tyranron

I thought about that, and also letting the user configure the version used. However, there are some adaptations that were necessary for GraphiQL to work with Juniper. I don't think it's possible to just grab the latest version from npm and use their index.html.

I do, however, agree that it's much nicer to bundle GraphiQL and GraphQL Playground for that matter instead of relying on a CDN. Blindly loading from an external source opens users up to certain risks, including privacy compliance within the Eurpoean Union.

What do you think would be the best way to keep the adaptations while delegating as much other code used to the maintainers of GraphiQL instead of maintaining a separate version? This applies specifically to React. Now it's hardcoded but the latest version of GraphiQL is loaded - not optimal.

@tyranron
Copy link
Member

@Empty2k12 grabbing the things only we need (index.html) and relying anything else to CDN is OK, imho. There will be too much hassle and unnecessary code bloat in bundling all the dependecies directly into the binary.

However, there are some adaptations that were necessary for GraphiQL to work with Juniper.

They can be applied in sed manner on-fly in post-install and adjusted by hand, if necessary.

The main point is making GraphiQL updates noticed, and so leverage dependabot and GitHub security advisories for the used versions. Updating those versions won't be something hard.

@Empty2k12
Copy link
Contributor Author

Empty2k12 commented May 26, 2022

They can be applied in sed manner on-fly in post-install and adjusted by hand, if necessary.

Not sure how long this will scale though, at some point some update requires another react, or a different API and then this method is going to break easily.

The main point is making GraphiQL updates noticed, and so leverage dependabot and GitHub security advisories for the used versions.

I agree that this is a good idea. However, I think it will be hard managing the correct versions of dependencies, like when do we update to React 18? GraphiQL will probably need it in version 2.0 but Dependabot will want to update it now.

There will be too much hassle and unnecessary code bloat in bundling all the dependecies directly into the binary.

Might be worth extracting GraphiQL and GraphQL Playground into helper crates juniper_graphiql and juniper_graphql_playground to allow users to keep their applications slim by only including them and the HTML/JS source when actually needed.

That however, seems like a choice up to the user. If there are specific requirements that prevent the usage of a CDN, they can always easily bundle and embed GraphiQL themselves.

@tyranron
Copy link
Member

tyranron commented May 26, 2022

@Empty2k12

However, I think it will be hard managing the correct versions of dependencies, like when do we update to React 18?

I don't think we need require React directly. Let it be a transitive dependency of GraphiQL. It's only enough to require GraphiQL, so dependabot will report about GraphiQL version changes only. And we just pick the version of React GraphiQL depends on.

That however, seems like a choice up to the user. If there are specific requirements that prevent the usage of a CDN, they can always easily bundle and embed GraphiQL themselves.

Yep. One can always implement his own GraphiQL endpoint if he has such strict requirements regarding CDNs. At the end of the day, it just a development helper tool, not a production quality client facing playground.

@Empty2k12
Copy link
Contributor Author

I have taken some time to look at the graphiql npm package, and I don't think what you're thinking is possible. The npm package does not contain any HTML files, just the bundled JS code. Therefore, we'd still need to ship our own HTML file (or shallow-clone graphql/graphiql and use their example HTML). Using the npm package is only useful when we're trying to lock the GraphiQL version by using the minified js from the npm package tied to the lastest merged version in this repo.

@tyranron
Copy link
Member

@Empty2k12 if it's missing in the package we can get it directly from GitHub by the package version in post-install script and post-process as we need.

https://github.com/graphql/graphiql/blob/graphiql%401.9.3/examples/graphiql-cdn/index.html

@Empty2k12
Copy link
Contributor Author

What is the point of pulling it via npm? We're not planning on using the bundled JS from the npm package. Just to hack Dependabot?

@tyranron
Copy link
Member

@Empty2k12 yes. Just to have tracked and streamlined versions management. Manual adjustments won't be the burden once the dependabot creates PR.

@tyranron tyranron marked this pull request as draft May 31, 2022 11:59
@everdrone
Copy link

Is there any new development on this one? Is the issue tracked somewhere else?

@LegNeato
Copy link
Member

LegNeato commented Aug 28, 2023

I think we should merge this. While it doesn't make us end up where we want to be, it's better than the current state. I'll land of CI passes.

@LegNeato
Copy link
Member

Failure is unrelated.

@LegNeato LegNeato marked this pull request as ready for review August 28, 2023 21:16
@LegNeato LegNeato merged commit b375146 into graphql-rust:master Aug 28, 2023
@Empty2k12 Empty2k12 deleted the feat/up-graphiql branch August 28, 2023 21:58
@tyranron tyranron added this to the 0.16.0 milestone Sep 12, 2023
tyranron added a commit that referenced this pull request Sep 13, 2023
- track GraphiQL new version via @dependabot
- automate GraphiQL integration glue adapting for new versions
- rework `example/warp_subscriptions`  to support subscriptions in new GraphiQL
tyranron added a commit that referenced this pull request Sep 13, 2023
- track GraphQL Playground new versions via @dependabot
- automate GraphQL Playground integration glue adapting for new versions
@tyranron tyranron added k::dependencies Pull requests that update a dependency file javascript Related to Javascript specifically labels Nov 20, 2023
@tyranron tyranron added the lib::graphiql Related to GraphiQL integration label Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement of existing features or bugfix javascript Related to Javascript specifically k::dependencies Pull requests that update a dependency file k::integration Related to integration with third-party libraries or systems lib::graphiql Related to GraphiQL integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants