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

Improve GraphiQL v2 #1182

Merged
merged 5 commits into from Dec 23, 2022
Merged

Improve GraphiQL v2 #1182

merged 5 commits into from Dec 23, 2022

Conversation

bramvanneerven
Copy link
Contributor

@bramvanneerven bramvanneerven commented Dec 22, 2022

Hi!

I am requesting some improvements to the GraphiQL v2 source generator.

What has changed?

  • The source generator now uses a templating solution (handlebars), which makes the generator more robust. We can directly pass the GraphiQLSource struct to generate the source, instead of doing a lot of 'manual' string based manipulations. This introduces a dependency, but I think it is worth it from a long-term maintenance perspective.
  • Changed credentials field to an enum. I think that is better than allowing arbitrary strings. [breaking change]
  • The endpoint and subscription_endpoint are now relative urls instead of absolute ones. This matches with how routes are defined in most web frameworks; Route::new().at("/", ...) in poem for example. This way, we do not have to know the exact origin url when developing the application, increasing the 'seperation of concerns'. [breaking change]

I also created a PR to update the examples: async-graphql/examples#64.

@sunli829
Copy link
Collaborator

Thanks! 👏

@sunli829 sunli829 merged commit b1b4bed into async-graphql:master Dec 23, 2022
@floric
Copy link
Contributor

floric commented Jan 7, 2023

Hey, I just noticed the new dependency (handlebars) from 5.0.5. As I never use GraphiQL, can we make this optional as a feature flag to keep the release smaller? If there is nothing against it, I could create a MR. :)

@sunli829
Copy link
Collaborator

sunli829 commented Jan 8, 2023

Yes,welcome to PR😄

@floric
Copy link
Contributor

floric commented Jan 8, 2023

Yes,welcome to PR😄

ok, will give it a try :)

@floric
Copy link
Contributor

floric commented Jan 8, 2023

Yes,welcome to PR😄

see #1202 :)

I wasn't sure if we should set it as a default feature or use opt-in as done in this PR. But I assume, users will be experienced enough to enable the feature if needed. Most likely most will stick to the ready to use integrations and there it's not used right now. Should be still a minor change though.

EDIT: Oh, I just saw I most likely will need to adapt the examples similar to this PR (async-graphql/examples#64), depending on your decision regarding the default feature.

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.

None yet

3 participants