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

added support for use_function_syntax_for_execution_context #3407

Conversation

kanodia-parag
Copy link
Contributor

What happened?
With very large type systems gqlgen can generate code that can no longer by compiled by the go compiler. Once more than 65.000 methods have been added to the executionContext the Go compiler fails with:

<autogenerated>:1: internal compiler error: too many methods on *executionContext: <some number>
Proposed Solution
Generate regular functions instead of receiver methods of *executionContext and pass *executionContext as function argument controlled by config - use_function_syntax_for_execution_context

Addressing this issue - #2681

@coveralls
Copy link

coveralls commented Dec 5, 2024

Coverage Status

coverage: 73.431% (-0.8%) from 74.202%
when pulling 4108703 on uber:kanodia-parag/usefunctionsyntaxforexecutioncontext
into 3114065 on 99designs:master.

@StevenACoffman
Copy link
Collaborator

@kanodia-parag Would you mind updating this a bit? There was some other PRs that landed recently that require some tweaks to this PR.

We deprecated the NewDefaultServer as it gives new people a false sense that they can just go to production with that config, and they instead really need to change things, especially if they want to use websockets or HTTP SSE. You can see how to update your tests here: ca4954c

Also, there's a conflicting fix that landed for the annoying test coverage reporting situation by excluding generated files more aggressively.

@kanodia-parag kanodia-parag force-pushed the kanodia-parag/usefunctionsyntaxforexecutioncontext branch from 8974433 to 2fbba69 Compare December 9, 2024 09:21
@kanodia-parag
Copy link
Contributor Author

@StevenACoffman I have updated the PR by rebasing it and fixing the linting issues coming from the test files.

@StevenACoffman StevenACoffman merged commit b9e4d51 into 99designs:master Dec 9, 2024
17 checks passed
@StevenACoffman
Copy link
Collaborator

Thanks for contributing this feature, especially with the tests!

Organizations with smaller scales may work around their pain points, while these become blockers like this at the scale of Uber. I hope you and your organization can contribute awareness (and possibly solutions) for problems that would otherwise go unreported.

For instance, it may be more obvious to your organization where we can best improve our performance to the benefit of all.

Please let us know if you encounter any other pain points, as it's extremely valuable! Looking forward to your next 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.

3 participants