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

More hygenic version of styled_components #48

Closed

Conversation

WorldSEnder
Copy link
Collaborator

@WorldSEnder WorldSEnder commented Sep 17, 2021

The current version of styled_components leaks the identifier __stylist_style_manager__. This can't be worked around without ditching the design and switching to a proc macro declaring other proc_macros, since __css_yew_impl has to have that identifier in scope.

In any case, I don't see a reason to not use function_component directly and have a hook-like macro that introduces the scoped versions. That's what use_stylist!(css); does: get the manager with use_context, then introduce macro_rules that are ready to use at the call-site, that refer to that manager (and the unmodifed stylist-macros).

There are a few hacks along the way that can be improved in the future, if the rust compiler follows accordingly.

  • The hacky ($dol:tt) to expand to a literal dollar sign in a proc macro comes from this issue but when RFC 3086 gets merged, this gets simplified to just using $$.
  • The whole macro rule is in a separate crate. Unfortunately, to export it from stylist, if it was defined in stylist, it needs a #[macro_export] slapped onto it, which also makes it visible from the crate root. Trying to hide it there in the docs with doc(hidden) also hides the re-export. If it's okay with you, I'd accept the re-export at the crate root, maybe with a textual warning in the docs?

There's also goodies to get

  • Allow the user to chose the name of the macro. Simple as use_stylist!(css as stylist_css) or similar.
  • No need to track yew-upstream for changes to the function_component macro to mirror

@WorldSEnder WorldSEnder marked this pull request as draft September 17, 2021 02:47
@WorldSEnder
Copy link
Collaborator Author

Macros still need test cases for style! and global_style! and the impl is probably wrong, cause with_manager doesn't yet exist for those.

@futursolo
Copy link
Owner

futursolo commented Sep 17, 2021

Thank you for the PR.

However, I don't think use_stylist(css) is a good replacement of styled_component.

Consider the following two designs:

use yew::prelude::*;
use stylist::yew::styled_component;

#[styled_component(Comp)]
pub fn comp() -> Html {
    html! {<div class={css!("color: red;")}></div>}
}
use yew::prelude::*;
use stylist::yew::use_stylist;

#[function_component(Comp)]
pub fn comp() -> Html {
    use_stylist!(css);
    html! {<div class={css!("color: red;")}></div>}
}

In my opinion, use_stylist!(css); is counter intuitive.

Despite I understand what the code above does, when seeing the code above, my mind tells me that it's invoking a function call taking a variable css instead of some kind of assignment.

Even the following does not mess with my mind as much.

use yew::prelude::*;
use stylist::yew::managed;

#[function_component(Comp)]
pub fn comp() -> Html {
    #[managed]
    use stylist::css;
    html! {<div class={css!("color: red;")}></div>}
}

This also applies to other crates use a syntax like this to declare stuff which I wish to avoid to introduce a similar usage.

Why #[styled_component(Comp)] over other designs?

This is an ergonomic design that adds 0 lines of code over standard function_component.

All other ways I can think of introduce at least 1 line of code for each component which is an unnecessary overhead to pay compare to this design.

In addition, styled_component is very iconic and will leave a deep first impression to users which is a deliberate choice I made when making this API.

leaking __stylist_style_manager__ is unhygienic.

This is true. But I am happy to overlook this issue at the moment this because it's very unlikely to collide this variable name unless the user is deliberately doing so.

Additionally, this can be solved with Span::def_site() once that becomes available in stable Rust.

style! and global_style! do not support with_manager.

This is by design.

Users should use APIs in stylist::yew::* over other APIs when using stylist with Yew.

I don't think use of style! and global_style! is justified when css! and <Global /> is available and more ergonomic to use.

Emotion also make a similar distinction on @emotion/react and @emotion/css where one is context aware and the other isn't.

Allow the user to chose the name of the macro. Simple as use_stylist!(css as stylist_css) or similar.

A similar effect can be achieved by #[styled_component(Comp, rename_css = styled)].
(not implemented at the moment, as I think it's not critical to include in the first round.)

No need to track yew-upstream for changes to the function_component macro to mirror

The current procedural macro accepts any valid function and passes it to the upstream function_component attribute.
I think it's unlikely that it's going to be changed to something other than a function.

In addition, a #[styled_component_base] attribute can be added to provide a usage in combination with #[function_component(Comp)] which means they can be made completely API neutral or use in combination with other libraries.

In addition, if users feel styled_component is too magical (or unhygienic), they can opt for the hook based design mentioned in #23 which is not landed at the moment.

@WorldSEnder
Copy link
Collaborator Author

WorldSEnder commented Sep 17, 2021

This is an ergonomic design that adds 0 lines of code over standard function_component.

My biggest concern: it starts getting unergonomic once another library adopts a similar design and wants to replace function_component, too. The two results don't compose and the user will have to chose between one or the other. I posit we aren't large enough to argue that our replacement is the one true one. A local use_* macro is perfectly composable with an arbitrary amount of libraries at hand.

In my opinion, use_stylist!(css); is counter intuitive.

Despite I understand what the code above does, when seeing the code above, my mind tells me that it's invoking a function call taking a variable css instead of some kind of assignment.

Alternate syntax use_stylist!(use css); should be more intuitive in that regard.

A similar effect can be achieved by #[styled_component(Comp, rename_css = styled)].

Reading code written with the current styled_component, which introduces an accessible name, doesn't make that clear. I have not seen other macros I use do that, and it's discouraged with macro_rules!. Sorry that I can't find the relevant section from the rust guide that talks about this, but I don't think it's wise to introduce names the user never mentioned, no matter how big a warning is in the docs of styled_component. When you only see the code in a diff of a larger project, it can get confusing.

style! and global_style! do not support with_manager. This is by design.

Why? Both Style and GlobalStyle have a new_with_manager and should insert the style at the closest (shadow) DOM root. I don't see the design choice that they aren't context sensitive. style! and global_style! might as well be supported.


In fact, I think that using the "global" versions of the macros in any component makes that component less usable than it could be and can cause bugs downstream, but that's for another discussion.


To close, I don't think saying "Add use_stylist!(use css); to your function_components" is harder to teach than "Replace your function_components with styled_components".

@futursolo
Copy link
Owner

futursolo commented Sep 17, 2021

My biggest concern: it starts getting unergonomic once another library adopts a similar design and wants to replace function_component, too.

You can combine the usage with the standard function component attribute (and other #[attrs]) with a similar attribute #[styled_component_base] (not implemented at this moment).

#[styled_component_base]
#[function_component(Comp)]
pub fn comp() -> Html {
    html! {<div class={css!("color: red;")}></div>}
}

Reading code written with the current styled_component introduces an accessible name, at all. I have not seen other macros I use do that, and it's discouraged with macro_rules!. Sorry that I can't find the relevant section from the rust guide that talks about this, but I don't think it's wise to introduce names the user never mentioned, no matter how big a warning is in the docs of styled_component. When you only see the code in a diff of a larger project, it can get confusing.

During the initial design, I have considered to introduce an equivalent stylist::yew::css! macro with the __stylist_style_manager__ named __this_macro_is_only_available_inside_of_a_styled_component__. But ultimately deemed better to just include it for the end user so it's not possible to accidentally import the wrong one.

I don't think there's ultimately a difference of introducing a custom syntax inside of a macro and and adding an variable as the same is achievable with replacing all css! with ::stylist::yew:: __css_yew_impl manually.

However, I think if the variable leaves the macro's control, then it's bad.

fn func() {
    declare_a!();  // bad, and shouldn't be possible?
    println!("{}", a);
}
#[declare_a] // Should be fine as custom syntax is allowed in this context.
fn func() {
    println!("{}", a);
}

Why? Both Style and GlobalStyle have a new_with_manager and should insert the style at the closest (shadow) DOM root.

I see this question as an opportunity cost.
Why would user use style! and global_style! if css! and <Global /> is both more powerful and simpler to use?
I really cannot think of any reason one would prefer style! and global_style! if using stylist with Yew.

@futursolo
Copy link
Owner

I forgot that Span::mixed_site() will achieve the same effect of isolating the variable from call site.

Will file an issue to improve this.

@WorldSEnder
Copy link
Collaborator Author

You can combine the usage with the standard function component attribute (and other #[attrs]) with a similar attribute #[styled_component_base] (not implemented at this moment).

I didn't know this could work. I'd actually prefer this way over the current implementation and repurpose this PR towards that. To make sure, #[styled_component_base] fn ... would work for any function, not just function_components, right? I can see the appeal of wanting to write helper functions to call from function components and use it there, too.

@futursolo
Copy link
Owner

repurpose this PR towards that.

Could you please close this PR and open a separate one if use_ is dropped so the history is not as messy?

To make sure, #[styled_component_base] fn ... would work for any function

This is possible with any function but I am not sure about the evaluation order of multiple attributes.
I think it evaluates from top to bottom which is why I wrote

#[styled_component_base]
#[function_component(Comp)]
pub fn comp() -> Html {
    html! {<div class={css!("color: red;")}></div>}
}

@WorldSEnder WorldSEnder deleted the hygenic_styled_comps branch September 17, 2021 14:01
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