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

Added register_local_helper_rc to RenderContext. #383

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ExPixel
Copy link

@ExPixel ExPixel commented Sep 26, 2020

Since register_local_helper and get_local_helper already make use of Rc in their signatures I figured there wouldn't be any harm in providing a way to create helpers using Rc directly without the reallocation required by Rc::from_box. It also allows using the same helper definition for multiple helpers with different names.

I also thought about fn register_local_helper<T: Into<Rc<dyn HelperDef + 'rc>>> but I think that might mess with type inference.

@sunng87
Copy link
Owner

sunng87 commented Oct 1, 2020

Thank you for your patch! Instead of creating a new method for this, I'm +1 for your idea of using generics. I'm working on 4.0 so even a little break change is acceptable at the moment.

Also implemented `HelperDef` directly on Rc's and Arc's
so that they can be passed directly into `register_local_helper`.
With this change we can call `register_local_helper(helper)` instead
of `register_local_helper(Box::new(helper))`.
@ExPixel
Copy link
Author

ExPixel commented Oct 1, 2020

So I went ahead and just made it generic in a bit of a different way. This should still allow Rc<dyn HelperDef> (I had to implement that one manually) and Box<dyn HelperDef> but should also just allow passing closures or anything else that implements HelperDef. I can change render_helper's API and usages as well if this seems good.

@sunng87
Copy link
Owner

sunng87 commented Oct 2, 2020

This looks great to me! Please go ahead.

@ExPixel
Copy link
Author

ExPixel commented Oct 2, 2020

Alright, so upon further inspection (and after more sleep) this doesn't really do what I set out for it to. The Rc::from(T) call is just going to produce an Rc<Rc<T>>. Without specialization there isn't really a way for me to implement things in a way that will allow T where T: HelperDef, Box<T> where T: HelperDef and Rc<T> where T: HelperDef to be used without extra allocations for Box<T> in Handlebars::register_helper and for Rc<T> in RenderContext::register_local_helper. Requiring Into<Rc<dyn HelperDef>> works but it messes up type inference and makes passing Box::new(new_helper) require some inconvenient type gymnastics. Going to have to mess around with trait bounds a bit more to get something that works decently well.

@sunng87 sunng87 force-pushed the master branch 2 times, most recently from 7abfce6 to 4962d7e Compare June 4, 2021 16:55
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.

None yet

2 participants