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

sbt plugin rewrite #933

Merged
merged 19 commits into from
Jun 26, 2021
Merged

Conversation

blast-hardcheese
Copy link
Contributor

This PR adds a reimagined workflow for caliban-codegen-sbt, automatically generating clients from files placed in src/main/graphql

I included what I presume is what's required for updating the docs, if anything more needs to be done there I'm happy to follow up.

@blast-hardcheese
Copy link
Contributor Author

blast-hardcheese commented Jun 21, 2021

One curious consideration is how sbt scripted tests are able to interpolate the current scalaBinaryVersion into paths.

At some point in the future the target/scala-2.12/... in auto-generate's test may end up being somewhat of an issue, and we may need to alter the tests to just refer to auto-generated elements at compile-time instead of direct exists checks.

@ghostdogpr
Copy link
Owner

Actually the scripted tests don't really need to be duplicated, they slow down the build quite a lot already and we expect the same code to be generated in both cases. I think a simple one would be enough.

@blast-hardcheese
Copy link
Contributor Author

Yeah, that is quite surprising. Up from 13 minutes to 20 minutes certainly seems quite excessive. I wonder whether the penalty really is due to starting up an entire environment from scratch, so more gains would be made by merging the two existing tests into one, especially since it's more about existing of files instead of build.sbt settings more significantly

@blast-hardcheese
Copy link
Contributor Author

Also, maybe more broadly -- it may not be necessary to run scalafmt for these automatically generated source files, as they are never intended to be committed. At least on my machine, the startup penalty there is nontrivial, so it would benefit me personally. I don't believe it would significantly impact the tests here, though maybe a little bit.

@blast-hardcheese
Copy link
Contributor Author

One breaking change here is renaming CodegenPlugin to CalibanPlugin. The motivation is that an unscoped CodegenPlugin doesn't make sense in a larger project with multiple, collaborative code generators.

If you feel strongly about this, I can try to add a @deprecated alias to maintain the old name. Alternately, I can add migration notes or something

@ghostdogpr
Copy link
Owner

Yeah, that is quite surprising. Up from 13 minutes to 20 minutes certainly seems quite excessive. I wonder whether the penalty really is due to starting up an entire environment from scratch, so more gains would be made by merging the two existing tests into one, especially since it's more about existing of files instead of build.sbt settings more significantly

Yeah that would be nice to merge them if you know how to. Also I think the gitlab one slow so no need to run it twice.

@ghostdogpr
Copy link
Owner

Also, maybe more broadly -- it may not be necessary to run scalafmt for these automatically generated source files, as they are never intended to be committed. At least on my machine, the startup penalty there is nontrivial, so it would benefit me personally. I don't believe it would significantly impact the tests here, though maybe a little bit.

Maybe make it an optional param, that is true by default with the explicit command and false by default with the new way?

@ghostdogpr
Copy link
Owner

One breaking change here is renaming CodegenPlugin to CalibanPlugin. The motivation is that an unscoped CodegenPlugin doesn't make sense in a larger project with multiple, collaborative code generators.

If you feel strongly about this, I can try to add a @deprecated alias to maintain the old name. Alternately, I can add migration notes or something

Yeah the old name was unfortunate. Deprecating it would be nicer though, to ease the migration.

@blast-hardcheese
Copy link
Contributor Author

Maybe make it an optional param, that is true by default with the explicit command and false by default with the new way?

I'll time it to see whether it actually is slow, then open a follow-up PR

@blast-hardcheese
Copy link
Contributor Author

OK, out of curiosity I commented out scalafmt. Scripted test reduced in time from 70 seconds to 42 seconds. For auto-generated clients, we could delay scalafmt until the very end (since once scalafmt is fully initialized subsequent runs are fast), but the challenge here is calibanGenClient isn't really compatible with that.

@blast-hardcheese
Copy link
Contributor Author

I must admit, I don't fully understand the url generation workflow, so this is just my best guess based on my current experience in order to further the discussion.

What will happen is caliban or compile will trigger client generation, which will hit the URL and then the generated file will be cached until a full clean or just caliban:clean, at which point subsequent compile will re-fetch the schema from the URL. This strikes a reasonable balance between utility and convenience, offsetting the cost of actually fetching the URL.

Copy link
Owner

@ghostdogpr ghostdogpr left a comment

Choose a reason for hiding this comment

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

Looks great! I left a few remarks and questions, mostly around docs.

vuepress/docs/docs/client.md Outdated Show resolved Hide resolved
vuepress/docs/docs/client.md Outdated Show resolved Hide resolved
vuepress/docs/docs/client.md Show resolved Hide resolved
vuepress/docs/docs/client.md Outdated Show resolved Hide resolved
Copy link
Owner

@ghostdogpr ghostdogpr left a comment

Choose a reason for hiding this comment

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

Looking good, thanks!

I will merge but prevent the docs generation until the next release.

@ghostdogpr ghostdogpr merged commit cd02111 into ghostdogpr:master Jun 26, 2021
@blast-hardcheese blast-hardcheese deleted the sbt-plugin-revamp branch June 27, 2021 09:22
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.

2 participants