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

use "embed" in generated code #2119

Merged
merged 8 commits into from
May 5, 2022

Conversation

vikstrous
Copy link
Contributor

@vikstrous vikstrous commented Apr 23, 2022

I work in a codebase that uses gqlgen heavily for a large graphql monolith. There are 384k lines of code in the files generated by gqlgen. We check in all these files into git because it takes a long time to generate them (and because it's a lot simpler to work that way).

This PR changes the generated files to use the embed feature of go to reduce the size of the generated files and reduce the chance of having to resolve PR conflicts.

I think that the complexity of the change is fairly low and there is no down side to introducing it. I made sure it has reasonable test coverage. It should be backwards compatible and doesn't change any behaviour.

I have:

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

@StevenACoffman
Copy link
Collaborator

This is a great idea, but we definitely need more coverage around it. We support Go 1.16 and newer, and we haven't really taken advantage of embedding up until now.

@vikstrous2
Copy link

vikstrous2 commented Apr 24, 2022

Alright, here's how we get this mergable:

  • Schema filenames are currently relative to the place gqlgen is run, not the generated code directory, so we need to change the filenames to make them relative to the correct directory
  • The filesystem variable and function have to be conditionally included if there are any embeddable sources - so we basically need a list of embeddable and non-embeddable sources
  • Some of the examples have files that are not embeddable, so that's nice, but maybe we should add an extra test for schema files that are entirely outside the go module to catch any unexpected regressions
  • We should consider making the sources for "builtins" references to gqlgen library constants rather than including them in the generated code... It would be a nice bonus.

@coveralls
Copy link

coveralls commented Apr 25, 2022

Coverage Status

Coverage increased (+0.02%) to 74.706% when pulling 524ec52 on vikstrous:sources-embed into f8e837b on 99designs:master.

@StevenACoffman
Copy link
Collaborator

Hey, I'm waiting on you to tell me when you think this is mergeable to re-review it

@@ -147,6 +170,30 @@ func BuildData(cfg *config.Config) (*Data, error) {
// otherwise show a generic error message
return nil, fmt.Errorf("invalid types were encountered while traversing the go source code, this probably means the invalid code generated isnt correct. add try adding -v to debug")
}
aSources := []AugmentedSource{}
for _, s := range cfg.Sources {
wd, err := os.Getwd()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not happy with having this here, but it seemed like the most reliable way to build the full path to the sources. Is there a better way?

@vikstrous
Copy link
Contributor Author

The PR is reviewable now. Sorry for the delay. This has been a weekend project :)
I didn't do the extra work to make "builtin" sources references, but someone can add that as a follow up if it's useful for them.

@StevenACoffman StevenACoffman merged commit fb5751a into 99designs:master May 5, 2022
@StevenACoffman
Copy link
Collaborator

Thanks! I would like to have someone make "builtin" source references, but not holding this up for that.

@frederikhors
Copy link
Collaborator

I'm having trouble with this. I think we'll have A LOT of issues for this.

@vikstrous can you please PR ASAP for docs instruction for this to work?

For example I'm having coming from 0.17.5:

panic: codegen problem: schema\schema.graphql not availalbe

THIS IS A HUGE PROBLEM.

@frederikhors
Copy link
Collaborator

Maybe this is a problem with Windows, @vikstrous?

I see path like: //go:embed "schema\\schema.graphql".

@frederikhors
Copy link
Collaborator

@StevenACoffman this is a huge problem.

I agree with the need to have it - even soon - but it should be tested better and maybe used with a feature flag maybe in the config file.

In projects like the one where I'm applying it (where there are several source of *.graphql files) it has now become a mess and nothing works.

Basically it's a COMPLICATED breaking change that came with a patch.

I propose to release 0.17.7 as soon as possible with this commit turned off and then we can see how to release it properly.

If we don't release 0.17.7 we can't use the other important changes from 0.17.6.

Please.

@vikstrous
Copy link
Contributor Author

Thanks for letting me know about the bug. I created issues for you: #2189 #2190

@vikstrous
Copy link
Contributor Author

@frederikhors Let me know if this fixes the issue: #2191 I don't have windows and I don't have a way to test it.

@frederikhors
Copy link
Collaborator

I renamed manually all the \\ to / and it works.

So I think we should fix it this way.

@mtibben
Copy link
Member

mtibben commented May 24, 2022

Hey @vikstrous @StevenACoffman

I'm a bit surprised by the scale of this change (and potential for issues?) with minimal discussion on its merits. The PR description talks about this as a Proof of Concept, yet it has been merged.

The PR description doesn't explain what the purpose of this change is, who or what it benefits and why.

Have I missed a discussion that happened somewhere else?

@vikstrous
Copy link
Contributor Author

@mtibben You are not missing any other discussion, but note that the PR description hasn't been updated after the proof of concept was turned into a mergable PR.

Sorry for the issues this caused. TBH I didn't expect anyone to be using this on windows, but I see that with popular open source projects, you never know what people will do with your code.

I can add more context about the motivation. I work in a codebase that uses gqlgen heavily for a large graphql monolith. There are 384k lines of code in the files generated by gqlgen. We check in all these files into git be because it takes a long time to generate them (and because it's a lot simpler to work that way). The motivation behind this PR was to reduce the size of the generated files and reduce the chance of having to resolve PR conflicts.

I think that the complexity of the change is fairly low and there is no down side to introducing it. I made sure it has reasonable test coverage. Clearly I hadn't considered windows support, but that's just a bug that could have been introduced by any PR regardless of what it's changing. This didn't seem like a major change because it's backwards compatible and doesn't change any behaviour (I wasn't the one that decided to include it in a patch relaese, but I agree with the decision).

@mtibben
Copy link
Member

mtibben commented May 25, 2022

Cool thanks for the explanation @vikstrous. Do you think you could update the PR description with this summary and an accurate description of the changes, so that future devs can understand the change and motivations fully?

@frederikhors
Copy link
Collaborator

I agree with @vikstrous this is not a big issue (the Windows issue is fixed in master and ready to be released with next patch release).

Perhaps a more conservative approach would have avoided some stress: releasing more often (using minor releases maybe for thing like this) is useful to reduce issues like the one I had on Windows.

Anyway @vikstrous did you get to see #2198?

@StevenACoffman
Copy link
Collaborator

StevenACoffman commented May 25, 2022

@mtibben This PR worked flawlessly in our production codebase (on linux and locally on MacOS), and was a backwards compatible change (with a bug on windows that went unnoticed because we weren't running tests there).

Backwards compatible changes seem ok to put in patch releases to me, but I'm open to discussing it. I don't think I've ever articulated my approach, so it's good to talk about it.

While semver implies Major.Minor.Patch => Breaking.Feature.Fix because of how go modules work, we aren't releasing a gqlgen V2, V3, V4, nor do we have the volunteer resources to maintain in parallel versions beyond whatever the latest release is. The V0 implies gqlgen may occasionally (and with great hesitation) break compatibility in minor ways between releases, but not for Patch releases. This effectively means that our releases are tagged according to v0.Breaking.(Feature|Fix)

We can, of course, discuss if this release tagging policy I've been following is ok to continue with.

@mtibben
Copy link
Member

mtibben commented May 26, 2022

@StevenACoffman yeah it would be great to get visibility of production systems testing these changes. I'd love for our systems here at 99 to also be on bleeding edge to build confidence (this is what google does with go), this would be something our @99designs/delivery-systems team may be able to think about the practicalities of.

As mentioned above I also think our PR descriptions could just be levelled up to provide accuracy. The PR description here still says this is proof of concept, when it seems to have moved beyond that - I'll try to fix that up now

@mtibben
Copy link
Member

mtibben commented May 26, 2022

@vikstrous I have updated the PR description now, can you confirm that it is accurate?

@vikstrous
Copy link
Contributor Author

Thanks! I removed the last part. Looks good now

@StevenACoffman
Copy link
Collaborator

@vikstrous if you could look into #2204 I would appreciate it.

@vikstrous
Copy link
Contributor Author

vikstrous commented Oct 11, 2022 via email

@vikstrous
Copy link
Contributor Author

vikstrous commented Oct 11, 2022 via email

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.

7 participants