[Yew integration] Styles cleanup on component redraw #22

vskh · 2021-01-24T18:09:30Z

Hi lukidoescode!

Thank you for this crate, I find it very useful and like the approach coming from css-in-js in React world.

I am facing the following problem: it seems that the stale styles added to page head by the component rendering are not being removed/replaced when component gets redrawn. This leads to increasing number of nodes in the page and eventually it slows down.

I use Yew integration as suggested in the example and not sure if this is a problem with css_in_rust or yew itself? Can I somehow clean it myself?

impl Component for MyComponent {
    type Message = ();
    type Properties = MyComponentProps;

    fn create(props: Self::Properties, link: ComponentLink<Self>) -> Self {
        let style = Style::create(
            "MyComponent",
            r#"
                width: 100%;
            "#)
            .unwrap();

        MyComponent {
            link,
            props,
            style
        }
    }

    fn update(&mut self, msg: Self::Message) -> ShouldRender {
        false
    }

    fn change(&mut self, props: Self::Properties) -> ShouldRender {
        let should_render = self.props.neq_assign(props);

        should_render
    }

    fn view(&self) -> Html {
        html! {
            <div class=self.style.clone()>
                {"Hello!"}
            </div>
        }
    }
}

Thanks for any suggestions.

EndilWayfare · 2021-04-10T00:26:14Z

I was looking into this crate for Yew styling as well, and I noticed the same issue.

Any time Style::create is called, a new <style> element is unconditionally created and appended to <head>.

css-in-rust/src/style/mod.rs

Line 85 in 51926c7

new_style = new_style.mount();

There is a lazy_static global registry that stores all created styles,

css-in-rust/src/style/mod.rs

Lines 19 to 21 in 51926c7

    
           lazy_static! { 
        
               static ref STYLE_REGISTRY: Arc<Mutex<StyleRegistry>> = Arc::new(Mutex::default()); 
        
           }

css-in-rust/src/style/mod.rs

Lines 91 to 93 in 51926c7

    
           (*style_registry) 
        
               .styles 
        
               .insert(new_style.class_name.clone(), new_style.clone());

but it is never accessed/modified anywhere else. Perhaps this is the beginnings of such a feature that hasn't been fleshed out yet?

I think a limitation of this design is that all Styles created are identified by their CSS class name and a random slug. This is good, because multiple Styles created in decoupled modules can't conflict on their class name. However, there's no way for the code to tell when multiple Style::create invocations are coming from the same client site (here, MyComponent::create).

Maybe there should be a macro that binds to its source location? Well, that wouldn't allow for dynamic styles...

Maybe the classname slug should depend on the hash of the style's contents? Maybe combine with source-location-macro to guard against collision?

I haven't done nearly enough research on CSS-in-JS solutions for implementation reference, I'm just spitballing

EndilWayfare · 2021-04-10T00:35:51Z

The concrete Style instance is never used by client code for anything other than its class_name string. Maybe instead of Style::create returning a full Style instance (as a "constructor with side-effects") it just returned the class_name? This might go some ways to un-hide the fact that it's a kind of get_or_insert operation.

EndilWayfare · 2021-04-10T00:54:06Z

Or maybe return some kind of StyleHandle with a Drop implementation that communicates with the registry? Up to this point, I was thinking solely in terms of avoiding duplicate inserts. If you could establish the identity of a create call, as suggested earlier, you could make sure that the registry (and by extension the <head>) has at most one copy of all creatable styles, lazily initialized as styles are required by components.

If the registry did reference counting, you could ensure that styles are removed from the document when no component refers to them anymore. This may not always be desirable, but it might keep larger SPA apps from bloating with styles that are only required, say, on one specific route.

Perhaps this could be configurable? If dropping a StyleHandle decrements the reference count to 0, the registry could remove the Style from the "live" map and either remove the <style> immediately or move the Style to a "garbage" map depending on a flag. If the Style gets recreated, check the garbage map first and move the Style back to the live map, no element creation or DOM manipulation required. A dispose_garbage public method could then handle the bulk removal of garbage at dev discretion (perhaps when navigating between sufficiently top-level routes). The "handle garbage immediately" flag could also be set by public method.

EndilWayfare · 2021-04-10T01:03:47Z

I'm saying "public method" here, because I assume that the current design continues to hide the global STYLE_REGISTRY behind public methods on the Style type. This is nice, because frameworks like Yew don't yet have a nice way to hand infrastructurish types around to components; Agents seem like serious overkill, which is fine because I don't think this is what they're designed for.

Maybe instead of a hard-coded lazy_static in the module, forcing a single implementation of the registry that has to handle all sorts of configurations, the registry becomes a first-class construct. Users would choose which flavor they want and put it in their own lazy_static. Ah, dang it, that would bust encapsulation for third-party component crates! They'd have to know about your style registry...

EndilWayfare · 2021-04-10T01:14:14Z

I don't mean to bust in and leave a pile of "here's some demands for some seriously complicated API changes ok bye!" I'm interested in helping out with implementation, if so desired. I'd wind up writing my own thing and spinning off into outer space with complicated features all by myself with nobody to sanity check against, otherwise. That sounds like a lot of duplicated effort and not much fun :)

@lukidoescode, I'd be interested in your thoughts. I didn't anticipate writing a novel, but these kinds of considerations have been following me around a lot recently.

lukidoescode · 2021-04-10T10:27:29Z

Hi @EndilWayfare, thank you for sharing your thoughts. You can always open up a PR and I will gladly integrate it. Please make sure you add yourself to the list of authors in there. I think the example isn't working because Yew is currently in rapid development and changing all the time. I still need to find a way to add the ability to work with multiple versions of Yew. I myself am too much a beginner with Rust for tackling this big of a task. This library is an idea of mine, a concept if you'd like. I'm currently trying to gather the financial resources for making it a real thing to be up to date all the time. Unfortunately until that time I have to rely on PRs to add functionality and for sometimes making css_in_rust up to date with Yew since I am unfortunately very constrained for time :(

EndilWayfare · 2021-04-10T22:09:05Z

Hey, @lukidoescode! Yeah, there's never enough hours in the day, I hear ya. I've never published an open-source crate; all my work is proprietary, internal company software.

As far as I can tell, this isn't Yew-specific. This issue should show up wherever a Style::create invocation at a particular point in the app's code is called more than once. If a component is created once, this problem won't happen even if the component is redrawn many times because the Style::create is in the constructor. Styles created in the root component will never be an issue. Styles created in a route "page" component will only be an issue when navigating away, then back onto the route. Styles created in more granular components, like a list item or something, will be an issue more often because the app is creating and dropping lots of them over time.

Just so I understand your design, is there supposed to be a place where styles are ever dropped or deduplicated?

lukidoescode · 2021-04-10T22:12:14Z

Oh, yeah, now I see. Thanks a lot for reporting! I will fix it as soon as I'll have some time left ✌🏼 It should of course remove unused style nodes. I'm curious about how future me will fix that 🤭😋

dylanowen · 2021-04-11T03:14:38Z

Hopefully this is helpful. I threw this code together with the intention of proving out the concepts from this discussion before writing up a real PR. I figured I should post it here in case I don't get a chance to actually write the PR though.

use std::collections::hash_map::DefaultHasher;
use std::collections::HashMap;
use std::fmt::Display;
use std::hash::{Hash, Hasher};
use std::rc::{Rc, Weak};

use web_sys::{Element, HtmlHeadElement};
use yew::utils::document;

thread_local! {
    static STYLE_REGISTRY: RefCell<HashMap<String, Weak<Style>>> = RefCell::new(HashMap::new())
}

pub struct Style {
    pub class_name: String,
    node: Element,
}

impl Style {
    pub fn new<P, C>(prefix: P, css: C) -> Rc<Style>
    where
        P: Display,
        C: ToString,
    {
        let raw_css = css.to_string();
        let hash = default_hash(&raw_css);
        let class_name = format!("{}-{}", prefix, hash);

        // lookup our style in the registry first
        let maybe_existing = STYLE_REGISTRY.with(|registry| {
            registry
                .borrow()
                .get(&class_name)
                .and_then(|reference| reference.upgrade())
        });

        if let Some(existing) = maybe_existing {
            existing
        } else {
            // create our new style
            let css = raw_css.replace("&", &format!(".{}", class_name));

            let css_node = document().create_element("style").unwrap();
            css_node.set_text_content(Some(&css));

            head().append_child(&css_node).unwrap();

            let style = Rc::new(Style {
                class_name: class_name.clone(),
                node: css_node,
            });

            STYLE_REGISTRY.with(|registry| {
                registry
                    .borrow_mut()
                    .insert(class_name, Rc::downgrade(&style))
            });

            style
        }
    }
}

impl Drop for Style {
    fn drop(&mut self) {
        let _ = head().remove_child(&self.node);
        // remove our weak reference from the registry
        STYLE_REGISTRY.with(|registry| registry.borrow_mut().remove(&self.class_name));
    }
}

fn default_hash<H: Hash>(element: &H) -> u64 {
    let mut hasher = DefaultHasher::default();
    element.hash(&mut hasher);
    hasher.finish()
}

fn head() -> HtmlHeadElement {
    document().head().unwrap()
}

Main Pieces:

I use the hash of the css to generate my uuid class-name this way we don't always need to recreate a style if there are duplicates. Ideally though this could be derived post-parsing so that differing whitespace/etc wouldn't impact the hash. (Also as you can see my "parsing" doesn't exist)
I use a Weak reference in my HashMap and always return an Rc, this way multiple pieces of my code can get a handle on a Style and it will be cleaned up once all the references are dropped.

EndilWayfare · 2021-04-13T00:26:35Z

Just glancing over it, that looks like a promising start! I may draft a PR of my own, whoever gets around to it first, I guess :)

Some thoughts:

Rc is probably fine. WASM isn't yet multithreaded. Even if (when) it gets there, actual rendering is still probably confined to one thread. Web workers shouldn't need access to the registry. Even if you did something like offload CSS parsing to a worker (unlikely to be worth serialization overhead), you could just hand the AST back to the main thread to interact with the registry. Similar rationale for non-WASM cases (like SSR, I suppose).
"Hash of the AST" definitely sounds like the way to go. Reintegrating the existing parse stuff shouldn't be too hard.
Ideally, there should be some protection against hash collisions. While probably exceedingly rare, the case where a new style hashed the same as an existing one, the new style would silently fail to create and the component would use the existing style. This has to be a well-researched problem (for dealing with distributed entity ids at scale), but it may be that "more perfect" solutions tend to be overkill. I could only see this being an issue on a very large project that uses more than one component library based on css-in-rust with very generic class names on the same page, or a very large number of dynamic style variations. The protection overhead would have to be very minimal to justify, I think. I don't have the probability chops to effectively analyze the situation exactly, but I'm tempted to conclude not worth it™

dylanowen · 2021-04-13T18:49:56Z

Thanks for taking a look @EndilWayfare .

That's a good point about Rc. For the non-WASM case we would Arc for each Style and Arc<Mutex<_>> for the overall registry, just like the original. We'd also need to use lazy_static instead of thread_local.

For the collision case, I agree with "not worth it™", I was also thinking that if the user knew they had a case like this, they can always change the prefix to Random to prevent this.

EndilWayfare · 2021-04-13T19:59:03Z

I think it's wise to keep code as similar as possible between platforms, deviating only where absolutely necessary or where there are significant "wins" (performance or otherwise). This should make testing easier, developer mental overhead lower, and "exported complexity" to dependent crates minimal.

There's precedent for maintaining parallel Rc/Arc implementations, like im-rs. But there, the smart-pointers are baked into the data structures that crate defines, not a singleton global.

The wasm-xxx-xxx targets seem to support atomics by lowering them to single-threaded equivalents.
https://github.com/rust-lang/rust/blob/5c1304205b7bc53a1e9f48cf286a60438351c1ab/compiler/rustc_target/src/spec/wasm_base.rs#L89-L91
I've heard this explained in terms of the atomic primitive types, so I don't know exactly how this extends to Arc<Mutex<_>>, but it seems to possibly be an option.

The other potential downside of thread_local is that the registry actually manages global state (the DOM), so users could potentially spin up competing registries in a way that's non-obvious. Answering their issues might require information that they don't know to provide. I'd like to err on the side of "non-surprising".

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Yew integration] Styles cleanup on component redraw #22

[Yew integration] Styles cleanup on component redraw #22

vskh commented Jan 24, 2021

EndilWayfare commented Apr 10, 2021

EndilWayfare commented Apr 10, 2021

EndilWayfare commented Apr 10, 2021

EndilWayfare commented Apr 10, 2021

EndilWayfare commented Apr 10, 2021

lukidoescode commented Apr 10, 2021

EndilWayfare commented Apr 10, 2021

lukidoescode commented Apr 10, 2021

dylanowen commented Apr 11, 2021

EndilWayfare commented Apr 13, 2021 •

edited

Loading

dylanowen commented Apr 13, 2021

EndilWayfare commented Apr 13, 2021

[Yew integration] Styles cleanup on component redraw #22

[Yew integration] Styles cleanup on component redraw #22

Comments

vskh commented Jan 24, 2021

EndilWayfare commented Apr 10, 2021

EndilWayfare commented Apr 10, 2021

EndilWayfare commented Apr 10, 2021

EndilWayfare commented Apr 10, 2021

EndilWayfare commented Apr 10, 2021

lukidoescode commented Apr 10, 2021

EndilWayfare commented Apr 10, 2021

lukidoescode commented Apr 10, 2021

dylanowen commented Apr 11, 2021

EndilWayfare commented Apr 13, 2021 • edited Loading

dylanowen commented Apr 13, 2021

EndilWayfare commented Apr 13, 2021

EndilWayfare commented Apr 13, 2021 •

edited

Loading