futursolo/stylist-rs

Turn *_from_sheet into Result<Style>

Closed this issue · 3 comments

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

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.

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?

  • 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.
  • If no, then style_a and style_b will be treated as different styles. How will the StyleKey look like?
    Do we key by elements? If so how to implement PartialEq and Eq and Hash for StyleKey? Especially given that it changes depending on mounting status.
    In addition, as web_sys::Element is !Sync and !Send, this would cause the current Arc<Mutex<X>> scheme to be unusable on the StyleRegistry unless we unsafe impl. I guess this is solvable by turning Registry to be thread local and have Rc<RefCell<X>> instead, but I didn't bother to experiment as that requires an additional dependency.
    Furthermore, the StyleRegistry is created to deduplicate mounting (and parsing in the past). Which kind of defects the purpose of it if it cannot reduce mounting effectively.

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())?;

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:

  • {precondition: Style is mounted or unmounted} mount() {postcondition: Style is mounted}. The Style gains one user.
  • {precondition: Style is mounted} unmount() {postcondition: Style might still be mounted by other users} The Style loses one users.

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 👍