Skip to content

fix(progenitor-macro): if 'client.inner' is not set by user do not provide it to user-specified functions. #933

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

Merged
merged 3 commits into from
Nov 25, 2024

Conversation

JosiahBull
Copy link
Contributor

@JosiahBull JosiahBull commented Oct 1, 2024

Description

When adding any hooks (pre_async, post, pre) you were implicitly required to add an empty type just to fill in client.inner property - even if you weren't actually using the client for anything. This mean that if you set an async hook without setting an inner client progenitor would create invalid code.

We have changed that behaviour to only generate the inner parameter if it's actually set by the end user.

Previous Code

// an inner type MUST be set to use with_pre_hook_async?!
// if we exclude this the generator succeeds but the generated code doesn't compile!
.with_inner_type(quote! { () })

// The actual hook...
.with_pre_hook_async(quote! {
    // We aren't even using the client.inner property!
    |_, request: &mut reqwest::Request| {
        Box::pin(async { Ok::<_, Box<dyn std::error::Error>>(()) })
    }
})

After Change

.with_pre_hook_async(quote! {
    |request: &mut reqwest::Request| {
        Box::pin(async { Ok::<_, Box<dyn std::error::Error>>(()) })
    }
})

Future Work

Given we have a token stream of the hook body, we could try to identify the parameters required and be a little smarter... but this is already a big improvement.

@upachler
Copy link
Contributor

upachler commented Oct 8, 2024

Thanks for providing this, I came across this problem myself last week (and I 'fixed' it by providing documentation in this PR #934, which I'm happy to adapt once yours gets accepted).

One thing to think about in future may be to move the inner type out of the scope of the code generator entirely, as well as the hook functions. So instead of generating a Client, which gets the inner type T and the hook functions /baked in/ via the generator, I'd rather like something like a Client<T> being generated where T is the inner type which may or may not implement certain traits like PreHook or PostHook.

This way, you can generate to client in a separte crate, but provide inner type and hook functions in the crate where you use them. This would allow to ship crates with a generated client that can then be augmented with inner type and hook function wherever it is used, without the need for a generator. Also I find hook development easier this way, because all the macro magic no longer gets in the way.

@ahl
Copy link
Collaborator

ahl commented Nov 25, 2024

Thanks for this. Regarding the idea of a type parameter to Client. Another thing to throw into the mix is that I was thinking about making a Client trait so that consumers could use something of their own creation... or add some interposition/middleware-type stuff.

@ahl ahl merged commit 54f21ac into oxidecomputer:main Nov 25, 2024
6 checks passed
upachler added a commit to upachler/progenitor that referenced this pull request Nov 26, 2024
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