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

Generating #[derive(TypedBuilder)] in another crate forces all crates under to add typed-builder as a dependency #109

Closed
awesomelemonade opened this issue Aug 8, 2023 · 6 comments

Comments

@awesomelemonade
Copy link
Contributor

See leptos-rs/leptos#1496. If Leptos updates to 0.15, it forces all leptos apps to add typed-builder to its dependencies in the Cargo.toml which is not ideal.

The easiest way to fix this is to revert this commit: b9ca287, which would mean each type would now have its own Optional again. This would yield more generated code.

Had a note here, but I guess it was forgotten about:
b9ca287#diff-f4519feb64908d4106578124d2bb7d0fbc21b38c3ae011d89aa95b2320feb7a8L170

Ideally, this should be resolved with rust-lang/rust#54363. There are potential workarounds here: frozenlib/parse-display#28, but that may require more effort to investigate and implement.

@awesomelemonade
Copy link
Contributor Author

Maybe it's the real issue of #101?

@idanarye
Copy link
Owner

idanarye commented Aug 9, 2023

This makes no sense. #101 was using a reexported TypedBuilder which messed up the crate paths, but this does not seem to be the case with Leptos. This is the only place I found where it uses TypedBuilder: https://github.com/leptos-rs/leptos/blob/3a98bdb3c2aee80db02bcb606e1e2bbf84cf080c/leptos_config/src/lib.rs#L26

It uses TypedBuilder directly from this crate - not reexported from elsewhere. So the code gets generated inside leptos_config/src/lib.rs, which has access to the typed builder crate. Shouldn't any crate that uses it just see a LeptosOptions type, that yes - has some (related) impls that use ::typed_builder::Optional, but how is that any different than other Rust crates that use stuff from other crates without reexporting them or asking their users to add them?

What am I missing here?

@awesomelemonade
Copy link
Contributor Author

Reverting "Add Optional" does fix leptos. See leptos-rs/leptos#1496 (comment). We could merge this patch in and re-unify Optional at a later time. Not sure what you think @idanarye

@idanarye
Copy link
Owner

I don't want to revert b9ca287. The real source is #31 - Optional is just the only thing that uses it right now, but the whole split is supposed to open the path for other features, like #95, which will also break Leptos in the exact same way once merged.

Also note that if someone else creates a macro that calls the Leptos macro, Leptos will have the exact same problem. Of course, it is much less likely that someone will do that with a web framework than with a coding sugar utility, but I never considered someone may use Typed Builder like this - so things may happen.

May I suggest a different approach? What if the TypedBuilder derive would accept the path to the typed_builder module? Usually it'd just be ::typed_builder, but:

#[derive(::leptos::typed_builder::TypedBuilder)]
#[builder(doc, crate_module_path=::leptos::typed_builder)]
#vis struct #props_name #impl_generics #where_clause {

Yes, it's more work, but only the crates that create macros will need to do that work, and these crates should alredy be prepared to do more work. Plus, if Leptos ever finds itself in the same boat, it'd add its own crate_module_path parameter (which will default to ::leptos) and do:

#[derive(#crate_module_path::typed_builder::TypedBuilder)]
#[builder(doc, crate_module_path=#crate_module_path::typed_builder)]
#vis struct #props_name #impl_generics #where_clause {

So this solution is transitive.

@awesomelemonade
Copy link
Contributor Author

awesomelemonade commented Aug 10, 2023

Yup your approach seems reasonable to me. serde and strum seem to do the same. Reverting was just a proof of concept/quickfix and obviously did not seem like a long term solution, which is why I never created a PR. I will likely be using my patch for now, but when I get some more free time (and it hasn't been fixed by then), I will make a PR adding crate_module_path similar to this

idanarye added a commit that referenced this issue Aug 14, 2023
idanarye added a commit that referenced this issue Aug 26, 2023
…_module_path

Close #109: Add `crate_module_path`
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

No branches or pull requests

2 participants