Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: @defer support #898
base: next
Are you sure you want to change the base?
feat: @defer support #898
Changes from 1 commit
cee50b7
4ad38b4
08ee6d2
35fdb02
d41fff6
274c5bc
3e3ca77
2b3c25a
369457e
780c668
b4d70fc
e27e5b0
6933420
d1d8485
2d599a7
ef73394
640300f
8b7e9b2
7a84f17
093d0df
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Please create an error in https://github.com/mercurius-js/mercurius/blob/master/lib/errors.js and use it here
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.
not needed, this is always true now.
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.
This should happen only if defer is enabled.
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.
I think we are not testing this behavior in CI, right? We should. The
peerDependency
might not be correct.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.
I could try adding CI tests that install
graphql@17
if you can point me to the right direction - how and where you add them?Also, what do you mean by
peerDependency not correct
? It didn't change in my PR, right?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.
How would an end user use defer? As you can see we depend upon
graphql
.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.
I see. So the Apollo Server, as a reference, didn't update graphql version and they will only do that when graphql@17 is released. Until that, it's the change that is implemented and tested on CI, but not available to the end users since it's still in alpha
That being said, the idea of mine was to do the same here - start supporting it but not allow users to use it until graphql@17 is officially released
In the future, when graphql@17 is live, we will update the mercurius dependency and everyone will be able to use defer
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.
I'm happy to cut a next branch for you, so we can land this with the alpha version of graphql-js.
wdyt?
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 to me! so i finish the feedback first and then you create a next release branch?
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.
+1
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.
I incorporated the feedback and going to see what's going on there with JIT compiler when I have some time
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.
move this for loop outside of the test definition
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.
Can you also add a test using undici.fetch()? It should work and support multipart responses.