futursolo / stylist-rs

A CSS-in-Rust styling solution for WebAssembly Applications
https://crates.io/crates/stylist
MIT License
370 stars 22 forks source link

Turn *_from_sheet into Result<Style> #14

Closed futursolo closed 3 years ago

futursolo commented 3 years ago

*_from_sheet is still fallible due to mounting. Currently it just panics which is a behaviour that might not be desirable.

WorldSEnder commented 3 years ago

I'd really prefer to instead pull mount out of the constructors or offer a different constructor without mount that does not fail and document that in the respective _from_sheet constructors.

futursolo commented 3 years ago

I experimented with the code for #11 by delegating mounting to a StyleManager, however, I realised that delayed mounting is causing complications on StyleRegistry.

I think first new from either string or Sheet should be mounted by default. As most of the time, this is the behaviour you want.

Currently, Style::new does the following:

  1. Parse css into Sheet if not sheet
  2. Register with cache and if exist in cache return cached instance
  3. Mount the style

If a Sheet is passed to an infallible method (say: new_from_sheet_unmounted), what does it do?

I think here neither 1 nor 3 applies. So the only thing it would do is 2. That is add / retrieve it to / from cache.

Consider the following code:

let sheet: Sheet = "color: red;".parse()?;
let style_a = Style::new_from_sheet_unmounted(sheet);
// ...
let style_b = Style::new("color: red;")?;
// ...
style_a.mount()?;

Are style_a and style_b the same Style instance in StyleRegistry?

With all these complications delayed mounting is going to create, is it worth to make such a change (and potentially introduce unsafe into the implementation) to cater a niche behaviour, especially given that delaying Style::new until it's ready to be mounted can avoid all the situation above?

Without delayed mounting, the manager API can be as easy as:

let mgr = IframeManager::with_selector("#my_frame");  // No holding of Element at the moment, because of `!Send` and `!Sync`.
// Each Manager also has it's own Registry, so it won't affect Styles mounted under other managers.
let sheet: Sheet = "color: red;".parse()?;
let style_b = Style::new_with_manager("color: red;", mgr.clone())?;  // Inner of Managers should be `Arc`'ed, so cloning here is as cheap as cloning an Arc.
// ...
let style_a = Style::new_with_manager(sheet, mgr.clone())?;
WorldSEnder commented 3 years ago

If yes, will the Style be mounted when style_b is created?

* If so, then the Style needs to be mounted at the time `style_b` is created. So `.mount()` does not guarantee a mount. what's advantage of having an unmounted Style over holding a Sheet than simply holding `sheet` and delay `style_a`'s instantiate until it mounts? Given that you still need to hold a `Style` instance to call the `.mount()` method. This design also will not allow a custom mounting point as a style can only be mounted once.

I would express mount/unmount as:

That is, mounting a Style guarantees that afterwards the Style is present and ready for use in html. Unmount tells stylist that it won't be needed until it's mounted again, but there might be other mounts of the same style still requiring it to stay mounted. Each manager for its own sounds like a good idea as well :+1: