-
Notifications
You must be signed in to change notification settings - Fork 72
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
Update dependencies #99
Update dependencies #99
Conversation
2e064b3
to
7956f6e
Compare
7956f6e
to
53b569b
Compare
Fix quart.request.get_data signature QuartClient -> TestClientProtocol
53b569b
to
730c796
Compare
DeprecationWarning: Use 'content=<...>' to upload raw bytes/text content.
"All versions of graphiql < 1.4.7 are vulnerable to an XSS attack." https://github.com/graphql/graphiql/blob/ab2b52f06213bd9bf90c905c1b460b6939f3d856/docs/security/2021-introspection-schema-xss.md
Was working by accident before
cannot collect test class 'TestClientProtocol' because it has a __init__ constructor
730c796
to
d1d8835
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great Job! The minimum version changes look reasonable, given the long time this library has been resting dormant.
Please see my comments attached below.
setup.py
Outdated
install_requires = ["graphql-core>=3.2,<3.3", "typing-extensions>=4,<5"] | ||
install_requires = [ | ||
"graphql-core>=3.2,<3.3", | ||
"Jinja2>=3.1,<4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is jinja now a required dependency? AFAIK it's only used for GraphiQL, which users can disable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup you're right. Just moved it back to test dependencies.
|
||
GRAPHIQL_VERSION = "1.0.3" | ||
GRAPHIQL_VERSION = "1.4.7" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to not update to 2.2.0
straight away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely should upgrade to 2.2.0. I was basing this on https://github.com/graphql/express-graphql/blob/main/src/renderGraphiQL.ts plus the graphiql readme recommending upgrading to 1.4.7 so I was under the impression that 1.4.7 is the latest version. Will try upgrading to 2.2.0 and verifying that everything's working, but maybe we should merge this first and I'll submit a subsequent PR for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
Push a few more commits to add python 3.11 and remove 3.6 (EOL) which I suspect is what makes the tests fail. |
ea3a4da
to
f292a7b
Compare
f292a7b
to
ff2d087
Compare
Update dependencies so that the package works with the latest framework versions in preparation for a stable v3 release. See #96 for context.
The PR's been rebased for convenient commit by commit review.
From
sanic
v21
view
now requires defining the methodget
,post
, ... independently otherwise will throw405
, thus the current approach of overridingdispatch_request
won't work. The workaround is to just assign the individual method todispatch_request
Close #86
Close #92
Close #94
Close #98
Supersede #87, #89, #91, #95.
@erikwrede