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

Args variables #468

Closed
wants to merge 5 commits into from
Closed

Conversation

kirach
Copy link
Contributor

@kirach kirach commented Dec 23, 2019

No description provided.

@yanns
Copy link
Contributor

yanns commented May 29, 2020

Was already tried & reverted:
sangria-graphql-org#10 (comment)

@dragisak
Copy link

dragisak commented Nov 2, 2020

What's the status of this PR ? Is it waiting for a patch from @kirach ?

@kirach
Copy link
Contributor Author

kirach commented Nov 2, 2020

Sorry guys, it's been on my todo list forever. We have a fork of sangria with the proper fix that we've been using for quite some time and it looks to be working fine. I just need to find some time to re-create another PR with the correct fix here.

cc @dragisak @yanns

@nickhudkins
Copy link
Contributor

Hey @kirach, I am happy to do the work if you don't have time. Is the working fork dv01-inc/sangria? If so I can rebase and get this tidied up. @yanns it looks like you had a application where this change exposed a defect, any chance you could explain how we could get a failing test case to ensure we don't accidentally re-introduce that?

@yanns
Copy link
Contributor

yanns commented Dec 9, 2020

@nickhudkins I don't have any minimal test for it.

@ibsmfar
Copy link

ibsmfar commented Jan 14, 2021

Hi! What's the status on this?

@nickhudkins
Copy link
Contributor

Hey @ibsmfar, while I am happy to work on this, this had been fixed in a PR, but then reverted due to an issue here:

sangria-graphql-org#10 (comment)

Unfortunately I don't know what it breaks, and we don't have a test that would show what breaks. I'll get this PR rebased, and maybe we could get a test release out the door to see if the breakage was elsewhere and since resolved.

@yanns you had commented that it broke a production application, was this an application you worked on? Is it a public repo that I could dig around in at least?

If not, I'll see if I can reproduce just by looking at the stack trace and query and seeing if that helps me get anywhere.

@yanns
Copy link
Contributor

yanns commented Jan 15, 2021

@yanns you had commented that it broke a production application, was this an application you worked on? Is it a public repo that I could dig around in at least?

It's not public.

@nickhudkins
Copy link
Contributor

#563 supersedes this PR

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

6 participants