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

Implement IntoView for Rc<str> #2462

Merged
merged 1 commit into from
Apr 5, 2024
Merged

Implement IntoView for Rc<str> #2462

merged 1 commit into from
Apr 5, 2024

Conversation

ydirson
Copy link
Contributor

@ydirson ydirson commented Mar 24, 2024

No description provided.

ydirson added a commit to ydirson/generals-familiar that referenced this pull request Mar 24, 2024
FIXME:
- more Rc::clone
- Relies on IntoView implementation for Rc<String> in leptos-rs/leptos#2462
ydirson added a commit to ydirson/generals-familiar that referenced this pull request Mar 24, 2024
FIXME:
- Relies on IntoView implementation for Rc<String> in leptos-rs/leptos#2462
@ydirson
Copy link
Contributor Author

ydirson commented Mar 25, 2024

Indeed as suggested on Discord, Option<str> is a much better choice than Option<String>

@ydirson ydirson closed this Mar 25, 2024
@ydirson ydirson reopened this Mar 25, 2024
@ydirson ydirson marked this pull request as draft March 25, 2024 20:15
@ydirson ydirson changed the title Implement IntoView for Rc<String> Implement IntoView for Rc<str> Mar 25, 2024
@ydirson ydirson marked this pull request as ready for review March 25, 2024 20:17
@gbj
Copy link
Collaborator

gbj commented Apr 1, 2024

Thanks for noticing this and your work to implement it! This implementation can be improved. Adding it to the viewable_primitives list will call .to_string() on the Rc<str>, allocating a new String with the same contents, and then converting that into an Oco<'static, str>. Oco<_> is a type we use internally that works like Cow<_>, but also has a reference-counted branch (so it can hold a &'static str, Rc<str>, or String)

You can convert an Rc<str> directly into an Oco<'_, str> without the additional .to_string() allocation and copy. So adding something like this, in the same file, right below the impl IntoView for Cow<'static, str> would be ideal!

impl IntoView for Rc<str> {
    #[cfg_attr(
        any(debug_assertions, feature = "ssr"),
        instrument(level = "trace", name = "#text", skip_all)
    )]
    #[inline(always)]
    fn into_view(self) -> View {
        View::Text(Text::new(self.into()))
    }
}

(note this is identical code to the Cow<_> implementation -- since both implement Into<Oco<_>> they are the same)

With-help-from: Greg Johnston <[email protected]>
Signed-off-by: Yann Dirson <[email protected]>
@gbj
Copy link
Collaborator

gbj commented Apr 5, 2024

Thanks so much! 🎉

@gbj gbj merged commit fc537c1 into leptos-rs:main Apr 5, 2024
61 checks passed
@ydirson ydirson deleted the intoview-rc branch April 5, 2024 21:59
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