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

xilem_web: Rewrite modifiers (Attributes, Classes and Styles), and cleanup/extend docs #699

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Philipp-M
Copy link
Contributor

Previously the modifier systems had design issues i.e. bugs (non-deleted styles/classes/attributes), and were unnecessary complex.
This aims to solve this (partly) by not using separate traits, but concrete types and a different mechanism that is closer to how ElementSplice works.

There's a few fundamental properties that composable type-based modifiers need to support to avoid surprising/buggy behavior:

  • Minimize actual changes to the underlying element, as DOM traffic is expensive.
  • Be compatible to memoization: e.g. a Rotate view should still be applicable to possibly memoized transform values of the underlying element.
  • Recreation when the underlying element has changed (e.g. with a change of variants of a OneOf).

To support all this, the modifier system needs to retain modifiers for each modifier-view, and track its changes of the corresponding view.
Previously all elements were directly written and separated with markers into a Vec to limit the boundaries of such views, but this had issues, when e.g. all modifiers were deleted (e.g. clearing a Vec of classes), by not reacting to this (I noticed that issue in the todomvc example with the footer).

With this PR, the count of modifiers of a modifier-view are directly stored either (hardcoded) in the view impl or its view state, which cleans up the concrete modifier elements (such as AttributeModifier, not including a separate Marker variant), and makes it less prone for errors (and is slightly less memory-intensive).

The API to use these modifiers in modifier-views was also redesigned to hopefully be more straight-forward/idiomatic. But as mentioned above there's still challenges, which introduce complexity (which I'd like to hide at least for simpler cases than these modifiers, likely in a future PR).
All of this should now be documented in the new modifier module, where now the modifiers Attributes, Classes and Styles reside. Other views (like events) may also end up there...

One interesting aspect compared to the previous system is the use of a new trait With for modifiers.
Instead of (roughly) Element: WithStyle, it works with Element: With<Styles>.
This prevents all kinds of reimplementations of something like WithStyle for elements.
This gets especially visible in the one_of module, which now can be covered by a single blanket implementation.

Further the cargo-feature "hydration" was deleted, as it causes more headaches to maintain than it really brings benefits (minimally less binary size), depending on the future, it may or may not make sense to reintroduce this.

Previously the modifier systems had design issues i.e. bugs (non-deleted styles/classes/attributes), and were unnecessary complex.
This aims to solve this by not using separate traits, but concrete types and a different mechanism that is closer to how `ElementSplice` works.

There's a few fundamental properties that composable type-based modifiers need to support to avoid surprising/buggy behavior:

* Minimize actual changes to the underlying element, as DOM traffic is expensive.
* Be compatible to memoization: e.g. a `Rotate` view should still be applicable to possibly memoized transform values of the underlying element.
* Recreation when the underlying element has changed (e.g. with a change of variants of a `OneOf`).

To support all this, the modifier system needs to retain a modifier for each modifier-view, and track its changes independently of the view.
Previously all elements were directly written and separated with markers, to limit the boundaries of such views, but this had issues, when e.g. all modifiers were deleted (e.g. clearing a `Vec` of classes), by not deleting these.

With this PR, the count of modifiers of a modifier-view is directly stored either in the view or its view state, which cleans up the concrete modifier elements, and makes it less prone for errors.

The API to use these modifiers in modifier-views was also redesigned to hopefully be more straight-forward/idiomatic.

One interesting aspect compared to the previous system is the use of a new trait `With` for modifiers.
Instead of (roughly) `Element: WithStyle`, it works with `Element: With<Styles>`.
This prevents all kinds of reimplementations of something like `WithStyle` for elements.
This gets especially visible in the `one_of` module, which can be covered by one blanket implementation.
Comment on lines +87 to +113
impl<T, A, B, C, D, E, F, G, H, I> AsMut<T> for OneOf<A, B, C, D, E, F, G, H, I>
where
A: AsMut<T>,
B: AsMut<T>,
C: AsMut<T>,
D: AsMut<T>,
E: AsMut<T>,
F: AsMut<T>,
G: AsMut<T>,
H: AsMut<T>,
I: AsMut<T>,
{
fn as_mut(&mut self) -> &mut T {
match self {
OneOf::A(e) => <A as AsMut<T>>::as_mut(e),
OneOf::B(e) => <B as AsMut<T>>::as_mut(e),
OneOf::C(e) => <C as AsMut<T>>::as_mut(e),
OneOf::D(e) => <D as AsMut<T>>::as_mut(e),
OneOf::E(e) => <E as AsMut<T>>::as_mut(e),
OneOf::F(e) => <F as AsMut<T>>::as_mut(e),
OneOf::G(e) => <G as AsMut<T>>::as_mut(e),
OneOf::H(e) => <H as AsMut<T>>::as_mut(e),
OneOf::I(e) => <I as AsMut<T>>::as_mut(e),
}
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This resulted from some experiments prior to the With trait, but I don't think it hurts to include this as well to xilem_core.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really like the existing AsRef implementation, but I've not spent too much time reasoning about it. That being said, this is fine to add this for consistency.

@Philipp-M
Copy link
Contributor Author

As this is a rather big PR (which got even bigger, as git hasn't recognized the move of files to the modifier module):
I did a fair bit amount of manual testing, including checks of regressions of wasm binary bloat (which is reasonably at ca. 1% average).
So I think a cursory review should be good enough.

I should probably add automated tests at some point. This system should be testable with unit tests without a browser.
If this system sticks and is able to be scale (i.e. add more modifiers like concrete properties of elements).

@Philipp-M Philipp-M requested review from DJMcNab and flosse and removed request for DJMcNab October 20, 2024 22:02
@burrbull
Copy link

Is it still tag(), tag().attr() and tag().attr().attr() are different types, requires OneOf to place them in different branches of if/match?

@Philipp-M
Copy link
Contributor Author

Philipp-M commented Oct 21, 2024

Is it still tag(), tag().attr() and tag().attr().attr() are different types, requires OneOf to place them in different branches of if/match?

Yes that was a deliberate decision a while back then, to make the API otherwise more consistent, and flexible (in other ways). I.e. make it fully declarative, to be able to use combinators, interchangebly (e.g. div(()).attr().class().attr()).

The user-facing API (apart of the modifiers) hasn't changed.

Way back I experimented with a different direction, but it felt a little bit hacky and is likely less efficient (div(()) is a ZST, or rather it was, until I had to add a boxing workaround, that may be removed when the next-trait-solver is finished), it may still be a direction, but not one I currently pursue.

We could add other modifier views though to make this a little bit different (and avoid recreation of the element with something like (OneOf)), something like this should be possible:

div(())
    .attrs((
        if true { Some(attr("key", "value")) } else { None },
        attr("other_key", 42),
    ))

But otherwise you could also do something like this currently, which should be similar to the above (though above I wouldn't expect that "key" deletes previous set values with the same "key"):

div(())
    .attr("key", if true { Some("value") } else { None })
    .attr("other_key", 42)

Copy link
Contributor

@flosse flosse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't really judge the content of the change.
I played around a bit in a sample project and couldn't find any problems from a user perspective.

@flosse flosse added the web label Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants