Skip to content

feat: add support for rendering GraphiQL with jinja #103

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 10 commits into from
Feb 15, 2023

Conversation

kiendang
Copy link
Collaborator

Render graphiql using jinja2 Template.render instead of regex search and replace.

Also supersede #97.

@kiendang kiendang force-pushed the fix-graphiql-render branch 3 times, most recently from 2547549 to 6f04dc4 Compare December 31, 2022 09:30
@kiendang
Copy link
Collaborator Author

Bump GraphiQL to 2.2.0
Screenshot 2022-12-31 at 22-51-40 GraphiQL

@erikwrede
Copy link
Member

New GraphiQL Version looks so much better! Is there any reason we need to drop the render_standalone? I find it quite handy to use if jinja isn't included as a dependency, and the normal jinja render can be used regardless.

@kiendang
Copy link
Collaborator Author

kiendang commented Jan 3, 2023

The biggest advantage of dropping the standalone render is that we would not need to maintain the json/js escaping logic, due to both security and complexity. I was trying to integrate #97 into simple_renderer and it just messed up the other graphiql tests.

I do see your point about not depending on Jinja. Will have another go at this.

@kiendang kiendang force-pushed the fix-graphiql-render branch 4 times, most recently from e05a0db to 2e7a8eb Compare January 6, 2023 04:21
Comment on lines 7 to 12
try:
from jinja2 import Environment
except ImportError:
pass
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is so that users don't need to install Jinja2 if they use the simple regex renderer. Kinda a dirty hack but should be safe since Environment is only imported here for type checking and we already did a runtime type check of jinja_env

Copy link
Member

Choose a reason for hiding this comment

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

that's fine for this case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This, #101 and #104 are ready for merging now.

@kiendang kiendang changed the title fix: use jinja built-in render fix: add support for rendering GraphiQL with jinja Jan 6, 2023
@kiendang
Copy link
Collaborator Author

kiendang commented Jan 6, 2023

All done now. The backslash escaping bug with the regex-based renderer (#97) has been fixed. By default the view uses the simple regex renderer (jinja_env set to None) to render GraphiQL and does not require Jinja2 to be installed. The jinja renderer is used if a jinja2.Environment is passed to jinja_env. jinja_env is also type checked during runtime.

@kiendang kiendang changed the title fix: add support for rendering GraphiQL with jinja feat: add support for rendering GraphiQL with jinja Jan 6, 2023
@kiendang kiendang force-pushed the fix-graphiql-render branch 2 times, most recently from e17d37f to 3484ca4 Compare February 7, 2023 03:31
Copy link

@elcharitas elcharitas left a comment

Choose a reason for hiding this comment

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

Amazing work done here @kiendang. I think what's left would be the code cov tests.

erikwrede
erikwrede previously approved these changes Feb 15, 2023
Copy link
Member

@erikwrede erikwrede left a comment

Choose a reason for hiding this comment

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

Great job! Codecov in this case is fine, as it's only complaining about the jinja import error checks, I'd be okay with wrapping these in a pragma nocov.

@kiendang kiendang force-pushed the fix-graphiql-render branch from 86c0c49 to a57d331 Compare February 15, 2023 11:52
@erikwrede erikwrede merged commit 8b9639e into graphql-python:master Feb 15, 2023
@kiendang kiendang deleted the fix-graphiql-render branch February 15, 2023 13:54
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.

3 participants