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

remove extra call to packages.Load fix #2505 #2519

Merged
merged 1 commit into from
Jan 26, 2023

Conversation

vikstrous
Copy link
Contributor

@vikstrous vikstrous commented Jan 23, 2023

This removes a slow and seemingly unnecessary call to packages.Load

I don't know how to test this because it's not clear what it's solving and how to reproduce the issue. In our codebase nothing bad happens, it just runs faster.

I have:

  • Added tests covering the bug / feature (see testing)
  • [N/A] Updated any relevant documentation (see docs)

@coveralls
Copy link

Coverage Status

Coverage: 75.529% (-0.04%) from 75.565% when pulling 098f337 on vikstrous:remove-extra-load into 9d22d98 on 99designs:master.

@StevenACoffman
Copy link
Collaborator

@vikstrous Can you verify that this does not cause an issue when following the example documentation and doing the go run github.com/99designs/gqlgen init ?

This code was added to fix #1765
in #1773

it is possible that it is not still necessary but I would like you to verify that. If it is necessary, perhaps there is a better way to fix the original problem that doesn't cause the redundant load for every other situation.

@vikstrous2
Copy link

vikstrous2 commented Jan 25, 2023

The quick start steps work with and without this PR.

If I comment out the autobind line (in the gqlgen.yml in the generated example) and/or add one that points to a package that doesn't exist, generate fails. With and without this PR.

This is the error:

reloading module info
generating core failed: unable to load example/graph/model - make sure you're using an import path to a package that exists
exit status 1

There was a test added in the linked PR and all tests are passing on this PR. I don't know which test suite it's in and there are a lot of them, but I'm guessing that whatever tests were added there are still running.

I'm really curious about "or just call packages.Load() twice" from #1767. I don't understand how calling load twice changes anything. It seems like @ipfans knows something I don't?


Re: the other reloads... It seems like all the extra calls to load are related to changes to packages caused by mixing up input and output packages. I want to dig into this more and see if I can remove more calls to load. Source: #1762 Maybe just making all calls to "ReloadAllPackages" more specific will do what I want. I'll look at that next.

@StevenACoffman StevenACoffman merged commit e6114a2 into 99designs:master Jan 26, 2023
@StevenACoffman
Copy link
Collaborator

Great! I appreciate your digging into this!

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.

4 participants