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

Refactor: Delegate all handling to core #152

Merged
merged 4 commits into from
Mar 13, 2024
Merged

Conversation

barelyhuman
Copy link
Contributor

@barelyhuman barelyhuman commented Feb 7, 2024

Changes

Test Modifications

  • All tests now use method and body type as supported by the core instead of re-implementing their own details
  • Modify tests to now have proper errors instead of coded anomalies

Upstream Dependency: mercurius-js/mercurius#1080
Closes #4

@simoneb
Copy link
Member

simoneb commented Feb 7, 2024

I like this approach more than the previous attempts 👌

@simoneb
Copy link
Member

simoneb commented Feb 7, 2024

Remind me again why we needed POST instead of GET and why now GET is fine?

@barelyhuman
Copy link
Contributor Author

barelyhuman commented Feb 7, 2024

Remind me again why we needed POST instead of GET and why now GET is fine?

I don't know why we needed POST, i thought there was a reason for it. I knew why the core was using POST though

Now it's fine since I found out that there were no specific reasons for the decision to use POST.
Since, the core registers both GET and POST requests and will handle persisted queries on both GET and POST, it's okay to do this.

Copy link
Member

@simoneb simoneb left a comment

Choose a reason for hiding this comment

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

LGTM, clearly we can't merge this until we land the upstream PR.

@barelyhuman barelyhuman mentioned this pull request Feb 7, 2024
4 tasks
@simoneb
Copy link
Member

simoneb commented Mar 12, 2024

@barelyhuman after updating the latest version of mercurius, which includes the change we needed, CI is failing. Not sure if you have a chance to look into this, but there must be something that was overlooked here.

@barelyhuman
Copy link
Contributor Author

It seems to be reusing the existing mercurius instance, a teardown should help. I'll check it out in a bit

@simoneb
Copy link
Member

simoneb commented Mar 12, 2024

It seems to be reusing the existing mercurius instance, a teardown should help. I'll check it out in a bit

Thanks 🙏

index.js Outdated
)
context: contextFn,
routes: true,
additionalRouteProps: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be additionalRouteOptions, I don't have push access to the repo anymore

Copy link
Member

Choose a reason for hiding this comment

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

ah thanks, good catch 👌

@simoneb simoneb merged commit 424c5a9 into master Mar 13, 2024
3 checks passed
@simoneb simoneb deleted the refactor/resync-with-core branch March 13, 2024 08:47
@github-actions github-actions bot mentioned this pull request Mar 14, 2024
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.

Support persisted queries
2 participants