Rewrite xilem_web to support new xilem_core #403

Philipp-M · 2024-06-18T09:29:14Z

This ports xilem_web to the new xilem_core.

There's also a lot of cleanup internally:

Get rid of all of the complex macros to support DOM interfaces, and instead use associated type bounds on the View::Element.
Introduce an extendable modifier based system, which should also work on top of memoization (Arc, Memoize) and OneOf views with an intersection of the modifiable properties.
- This modifier based system gets rid of the hacky way to propagate attributes to elements, and takes inspiration by masonrys WidgetMut type to apply changes.
- Currently that means Attributes, Classes and Styles to reflect what xilem_web previously offered.

Downsides (currently, needs some investigation):

Due to more internal type complexity via associated types this suffers from rust-lang/rust#105900. The new trait solver should hopefully mitigate some of that, but it seems currently it completely stalls in the todomvc example (not in other examples).
The deep, possibly completely static composition via associated type-bounds of the view and element tree unfortunately can take some time to compile, this gets (already) obvious in the todomvc example. The other examples don't seem to suffer that bad yet from that issue, probably because they're quite simple.

~~I really hope we can mitigate this mostly, because I think this is the idiomatic (and more correct) way to implement what the previous API has offered.~~

One idea is to add a Box<dyn AnyViewSequence>, as every element takes a "type-erased" ViewSequence as parameter, so this may solve most of the issues (at the slight cost of dynamic dispatch/allocations).

Edit: idea was mostly successful, see comment right below.

I think it also closes #274

It's a draft, as there's a lot of changes in xilem_core that should be upstreamed (and cleaned up) via separate PRs and I would like to (mostly) fix the slow-compile time issue.

…n between actual attributes

…nup a little bit

… as well

…not needed

… using boilerplatey functions

…cause of type-checking

Philipp-M · 2024-06-20T22:41:00Z

but it seems currently it completely stalls in the todomvc example

Just to clarify, as I think there was already some misunderstanding, this is only true for the new (unfinished) trait-solver, not for the current one. The todomvc example takes without the new changes explained below, about 5s to compile in release mode vs 0.75s with current xilem_web. It'll be interesting to see how the new trait-solver will handle this, when it's finished, maybe we can convert it back to being completely static/stack-based again then (wishful thinking...).

So indeed the idea of type-erasing the ViewSequence mostly fixes the compile time issue, which makes sense, since it chops down the otherwise completely static composition of the view tree at each element boundary, since every DOM element takes a ViewSequence as parameter. But as expected this results in regression of either the performance (very slightly, less concerning than binary size at least) as well as binary size unfortunately. Compared to old xilem_web this binary size change is about 5-10% bigger (the lower end when it's compressed). Though I noticed, the bigger the app gets, the less of an issue this is (but we still need evidence of really big apps...)

I do think it's worth it to have this kind of regression, with that we are roughly at the same compilation times as before and IMO the other advantages (disregarding the actual port to new xilem_core) outweigh the slight regression. I also think that it (the compile times) should scale this way, as the static type composition will not grow "infinitely". With incremental compilation, probably also big apps will compile somewhat fast. But that needs to be proven with bigger apps than the todomvc example...

I've also started documenting xilem_web. So every item directly visible at in docs.rs should now at least hint what's going on.

Philipp-M

So I think this is now in a good state for reviewing, I added more documentation (including simple doc tests in the interfaces, to test whether doc is up-to-date), it should now cover the more important parts of the architecture/API.
I have also added a new example (blog), which shows at least one thing that wasn't possible before (extending memoized elements).
The last things this is depending on is #402 and the new blog example depends also on #175, so after these are merged, this would be ready to merge as well (I'll do a merge from main into this branch before though).

As this is quite a big PR, and I doubt someone has the time or will to dive through all of this in detail while reviewing,
I'll give some hints via code-comments to the more interesting, and possibly discussion worthy things of it, as I still think this may be a reasonable direction for Xilem itself as well.

I guess, when there's use for it, I could also add some modified version of the comments as document such as ARCHITECTURE.md or something.

Philipp-M · 2024-06-23T20:23:21Z

xilem_web/src/interfaces.rs

+// #[cfg(feature = "HtmlAnchorElement")]
+pub trait HtmlAnchorElement<State, Action = ()>:
+    HtmlElement<State, Action, DomNode: AsRef<web_sys::HtmlAnchorElement>>
+{
+}


This whole file is mostly generated (with a local hacky webidl based generator).
I have avoided macros because I expect this to be continuously extended overtime, and as most of us, I'm not a fan of macros, as I think DX suffers.
I've added all these commented out feature gates because I think, when some of the element props are extended (e.g. a video element has then VideoElementProps which composes ElementProps + additional stuff relevant to the video element), that inevitably the wasm binary size will grow, if we don't constrain this with features.
Because I have not yet found a way that is reasonable and robust at least to do this just with static typing, such that these additional props are optimized away when they're not needed.
I don't think it's a good idea to let all the users suffer when they don't need such features.
Currently this is not necessary because as seen below, all is basically depending on the ElementProps and I think the Element interface/trait should be included by default.

Philipp-M · 2024-06-23T20:24:32Z

xilem_web/src/interfaces.rs

+    /// A string representing the value of the HTMLOptionElement, i.e. the value attribute of the equivalent `<option>`.
+    /// If this is not specified, the value of text is used as the value, e.g. for the associated `<select>` element's value when the form is submitted to the server.
+    ///
+    /// See <https://developer.mozilla.org/en-US/docs/Web/API/HTMLSelectElement/value> for more details
+    fn value(self, value: impl IntoAttributeValue) -> Attr<Self, State, Action> {
+        Attr::new(self, Cow::from("value"), value.into_attr_value())
+    }


My vision of the API is obviously builder/modifier-pattern like as seen with all these DOM interface extension traits, and the examples.
So I added all these attributes (seen in the blog example, which doesn't use any Element::attr) as methods, currently just sugaring the Attr view.
These are of course not complete, I only added the attributes needed in the blog example, and added MDN docs on top of some.
I expect this to be extended over time (together with the feature gates explained above).

Philipp-M · 2024-06-23T20:27:26Z

xilem_web/src/attribute.rs

+impl WithAttributes for Attributes {
+    fn set_attribute(&mut self, name: CowStr, value: Option<AttributeValue>) {
+        let new_modifier = if let Some(value) = value {
+            AttributeModifier::Set(name.clone(), value)
+        } else {
+            AttributeModifier::Remove(name.clone())
+        };
+
+        if let Some(modifier) = self.attribute_modifiers.get_mut(self.idx) {
+            if modifier != &new_modifier {
+                if let AttributeModifier::Remove(previous_name)
+                | AttributeModifier::Set(previous_name, _) = modifier
+                {
+                    if &name != previous_name {
+                        self.updated_attributes.insert(previous_name.clone(), ());
+                    }
+                }
+                self.updated_attributes.insert(name, ());
+                *modifier = new_modifier;
+            }
+            // else remove it out of updated_attributes? (because previous attributes are overwritten) not sure if worth it because potentially worse perf
+        } else {
+            self.updated_attributes.insert(name, ());
+            self.attribute_modifiers.push(new_modifier);
+        }
+        self.idx += 1;
+    }
+
+    fn start_attribute_modifier(&mut self) {
+        if self.build_finished {
+            if self.idx == 0 {
+                self.start_idx = 0;
+            } else {
+                let AttributeModifier::EndMarker(start_idx) =
+                    self.attribute_modifiers[self.idx - 1]
+                else {
+                    unreachable!("this should not happen, as either `start_attribute_modifier` happens first, or follows an end_attribute_modifier")
+                };
+                self.idx = start_idx;
+                self.start_idx = start_idx;
+            }
+        }
+    }
+
+    fn end_attribute_modifier(&mut self) {
+        match self.attribute_modifiers.get_mut(self.idx) {
+            Some(AttributeModifier::EndMarker(prev_start_idx))
+                if *prev_start_idx == self.start_idx => {} // class modifier hasn't changed
+            Some(modifier) => {
+                *modifier = AttributeModifier::EndMarker(self.start_idx);
+            }
+            None => {
+                self.attribute_modifiers
+                    .push(AttributeModifier::EndMarker(self.start_idx));
+            }
+        }
+        self.idx += 1;
+        self.start_idx = self.idx;
+    }
+}


This is one of the more interesting parts of this PR, a modifier based system (Styles, Classes and this Attributes), which is designed in such a way, that Memoize or Arc doesn't offer weird surprises (as well as just overwriting attributes, when they're previously set, which would have problems, when just directly setting attributes on the DOM elements).

As well as OneOf views being able to intersect on the elements and their props in a strong/statically-typed way. For example when there is OneOf2::A(html::div(())) and OneOf2::B(mathml::mrow(()), this OneOf2 view fulfills the contract of the Element trait, but not of the HtmlElement, because the trait bound AsRef<HtmlElement> is not satisfied for mathml::mrow, so e.g. HtmlElement::style is not possible to use (currently at least, because MathML should be able to support style, this needs extension in wasm_bindgen I think.), but all the methods of Element are possible on mathml::mrow as well.

The modifier system works in such a way, that it has stateful start and end methods, which add markers into a Vec for the modifier type on its props to know where the modifier view has started/ended.
Additional modifiers for this property (e.g. AttributeModifier::Set or AttributeModifier::Remove) are then pushed into this Vec until the next modifier starts.
All of this happens in either View::build as well as View::rebuild, and these attributes are added on every reconciliation (i.e. View::rebuild), so that it can detect changes (so all these attributes should be added always in the same order, to avoid unnecessary updates).
On each reconciliation pass, it is checked, whether the attribute at the same index has changed, and if so, the relevant props or attributes are marked dirty, such that they're correctly updated when applying the props to the actual DOM element.
This should be robust to underlying element changes in e.g. a OneOf which would build the new view, while being in a View::rebuild of a composing modifier view, as the actual modifiers have to be applied on the up-traversal and only the end method actually tells via an EndMarker where the modifier has started (which is read by the start method on the down traversal, to jump to the next modifier).

All the possible accumulated changes are then applied when SuperElement::upcasting the element, which usually happens on a ViewSequence boundary.
These modifiers are described via traits such as (currently), WithAttributes, WithClasses and WithStyles and are also working with the intersection of OneOf types.

DJMcNab · 2024-06-24T08:44:38Z

xilem_web/web_examples/blog/src/main.rs

I've not done much review, but the name of this example is concerning. If you're doing a blog, you really should be doing server side rendering, imo.

I'm not sure though, I think it's good to get another sizeable example.

I'm open for alternative suggestions, could also be something like "news" or what I'm probably planning is a simple hackernews implementation. I actually want to revisit the hydration support I've experimented with quite a long time ago. So I think in the future this likely will be extended with SSR (I think it's reasonable to support SSR in xilem_web isn't it?).

Yeah, SSR and hydration in Xilem Web would be incredible and is something we should support sooner rather than later, if possible.

I think reframing it in terms of e.g. a "ticker", so having the concept that the feed will be live-updating relatively frequently might be good enough. I don't want anyone getting the idea that we want people to be using Xilem Web as-is for a blog.

Yeah you're probably right, a blog is not the most interactive example and should mostly be SSR, at least short term this would be misleading. I'll update that example as you said, to something that is more interactive.

I was too lazy to add another example, removed it for now, I guess this somewhat "fixes" the issue. I may revisit it someday later though.

…ail, the API requires an `impl ViewSequence` instead (which is boxed) for the element builder functions

…ix doc comment issue

Philipp-M · 2024-06-27T17:17:26Z

All of the dependent PRs are merged, it got another minor cleanup round, I think it's ready to merge now.

DJMcNab

I have not reviewed any of the actual logic here, but found a few things on a very quick skim

DJMcNab · 2024-06-27T19:33:29Z

xilem_web/README.md

@@ -1,9 +1,32 @@
-# `xilem_web` prototype
+# `xilem_web`


We could plausibly follow the usual readme patterns here, but that probably isn't a job for this pr

You're right, I've copied a lot of the Xilem Core Readme and actually used the same pattern to include it in the lib.rs now, you probably want to look quickly at it again before I merge.

I'll update both once this is merged. Thanks!

xilem_web/README.md

xilem_web/src/app.rs

DJMcNab · 2024-06-27T19:36:22Z

xilem_web/src/app.rs


 /// The type responsible for running your app.
-pub struct App<T, V: View<T>, F: FnMut(&mut T) -> V>(Rc<RefCell<AppInner<T, V, F>>>);
+pub struct App<T, V: DomView<T>, F: FnMut(&mut T) -> V>(Rc<RefCell<AppInner<T, V, F>>>);


I'd obviously much prefer this avoided interior mutability, but I'll assume it's necessary - I don't have enough context to know whether it's avoidable

That's actually one of the few things from the old code-base that I haven't paid too much attention to, nor written it myself. But I do think it's necessary, since the App itself is "forgotten" to avoid blocking the event-loop, and just lives on within the view context (and message thunks).

DJMcNab · 2024-06-27T19:44:15Z

xilem_web/src/message.rs

+/// A dynamically typed message for the [`View`] trait.
+///
+/// Mostly equivalent to `Box<dyn Any>`, but with support for debug printing.
+// We can't use intra-doc links here because of


I guess I should fix this sentence fragment in both places...

Yeah, I'm not really sure what you mean with it^^ (I suspect it has something to do with dyn...)

Yeah; it's the dyn Message. Rustdoc doesn't seem to have a syntax to create an intra-doc-link to this item

xilem_web/src/message.rs

DJMcNab

A few tweaks on the comment feedback, but otherwise we should be good to land!

xilem_core/src/message.rs

xilem_web/README.md

DJMcNab · 2024-06-28T07:46:01Z

xilem_web/src/message.rs

+/// A dynamically typed message for the [`View`] trait.
+///
+/// Mostly equivalent to `Box<dyn Any>`, but with support for debug printing.
+// We can't use intra-doc links here because of


Yeah; it's the dyn Message. Rustdoc doesn't seem to have a syntax to create an intra-doc-link to this item

DJMcNab · 2024-06-28T07:46:52Z

xilem_web/src/lib.rs

+</style>
+
+<!-- Hide the header section of the README when using rustdoc -->
+<div style=\"display:none\">


This format of the README is out-of-date as compared to linebender/linebender.github.io#55

But I'll update both of them later, so this is still fine

…ociated type (#619) This was the initial plan of #403, I can't remember anymore why I didn't made it to work back then. Anyway this should simplify the type signature of `Pod` and `PodMut` a little bit. This also includes a little bit of refactoring otherwise (mostly aesthetic + removal of unnecessary wrapper `DynNode`)

Philipp-M added 29 commits June 12, 2024 21:22

xilem_core: Add ViewSequence::count

42ece98

xilem_core: Temporarily don't require Send bound on Message trait

ac6fe76

xilem_web: Initial commit of rewrite

918083f

Slight cleanup and rename of Attrs to Props for better distinctio…

605361e

…n between actual attributes

Add experimental OneOf2 View in xilem_core and xilem_web

fc614af

Small fix in classes, and more renaming

06232f2

xilem_web: Port counter example to new xilem_web

304b78d

Refine OneOf2 view (reduce boilerplate) add generational ids and clea…

c422e4f

…nup a little bit

Add a OneOf2 ViewSequence impl

56473df

Cleanup OneOf2 view and sequence and document everything

80c6213

Add tests for OneOf2

731094e

Add OneOf2-9 implementations via a macro

dc1e4e3

Add orphan views

4a3fa4b

A lot of fixes, and add DOM interfaces

40406bb

Port style support from old xilem_web and port the mathml_svg example…

f525a53

… as well

Add parent to PodMut and fix OneOf view impl with it

ed2cc6c

Lazy access to attributes, which increases performance, when they're …

ef2ccfa

…not needed

Cleanup with AnyPod alias, and reduce macro-expanded boilerplate by…

8f5efbf

… using boilerplatey functions

Ported Pointer view from xilem_web

71f6c85

Port Fill and Stroke views and add them to the relevant interfaces

8862d77

xilem_core: Add support for kurbo shapes

1e3175e

Port kurbo shapes and svgtoy example

b8bd42d

Port todomvc example

f91e4c8

Port custom_element

2092b85

Add missing Adapt view

7675762

Delete old xilem_web and rename new xilem_web2 to xilem_web

50b921a

Add a DomViewSequence as a workaround for regressing compile times be…

c732da5

…cause of type-checking

Cleaned up and documented a lot

1be6bfa

Merge branch 'main' into xilem_web2

c6e053e

Philipp-M commented Jun 23, 2024

View reviewed changes

DJMcNab reviewed Jun 24, 2024

View reviewed changes

Philipp-M added 8 commits June 25, 2024 19:46

Merge branch 'main' into xilem_web2

0ed9231

Fix CI

51f01ca

Remove blog example

d5e420d

Make the DomViewSequence a private (crate level) implementation det…

1b53a9b

…ail, the API requires an `impl ViewSequence` instead (which is boxed) for the element builder functions

Readd missing LICENSE to xilem_web

e538bd0

Move ViewCtx to a separate module context

6aa53ab

Add builder methods from old ViewExt trait to DomView trait and f…

ac8caa6

…ix doc comment issue

Add missing License header

40745c8

Philipp-M marked this pull request as ready for review June 27, 2024 17:18

Philipp-M added 2 commits June 27, 2024 19:22

Readd meta in Cargo.toml

4e29707

... and readd package.metadata.docs for scrape-examples in Cargo.toml

4eb1e1d

DJMcNab approved these changes Jun 27, 2024

View reviewed changes

Philipp-M added 2 commits June 27, 2024 23:32

Improve readme and use same trick as in Xilem Core

bdf4259

Use Rc<[ViewId]> instead of Vec<ViewId> where possible

05038d6

DJMcNab approved these changes Jun 28, 2024

View reviewed changes

Address Review Feedback

ca212a0

Philipp-M enabled auto-merge June 28, 2024 08:27

Philipp-M added this pull request to the merge queue Jun 28, 2024

Merged via the queue into linebender:main with commit b33a2a6 Jun 28, 2024
8 checks passed

Philipp-M deleted the xilem_web2 branch June 28, 2024 08:37

This was referenced Jul 24, 2024

xilem_web: Setting fill color on SVG stroke fails to compile #446

Closed

xilem_web: Add function to set HtmlIFrameElement src attribute #466

Merged

Philipp-M mentioned this pull request Aug 5, 2024

xilem_web: Add async_repeat #485

Merged

RagibHasin mentioned this pull request Aug 6, 2024

Fix non-standard textbox behavior in Windows #489

Merged

Philipp-M mentioned this pull request Sep 28, 2024

xilem_web: Refactor unnecessary generic type parameter Props to associated type #619

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite xilem_web to support new xilem_core #403

Rewrite xilem_web to support new xilem_core #403

Philipp-M commented Jun 18, 2024 •

edited

Loading

Philipp-M commented Jun 20, 2024

Philipp-M left a comment

Philipp-M Jun 23, 2024

Philipp-M Jun 23, 2024

Philipp-M Jun 23, 2024

DJMcNab Jun 24, 2024

Philipp-M Jun 24, 2024

DJMcNab Jun 24, 2024 •

edited

Loading

DJMcNab Jun 24, 2024

Philipp-M Jun 24, 2024

Philipp-M Jun 27, 2024

Philipp-M commented Jun 27, 2024

DJMcNab left a comment

DJMcNab Jun 27, 2024

Philipp-M Jun 27, 2024

DJMcNab Jun 28, 2024

DJMcNab Jun 27, 2024

Philipp-M Jun 27, 2024

DJMcNab Jun 27, 2024

Philipp-M Jun 27, 2024

DJMcNab Jun 28, 2024

DJMcNab left a comment

DJMcNab Jun 28, 2024

DJMcNab Jun 28, 2024

Rewrite xilem_web to support new xilem_core #403

Rewrite xilem_web to support new xilem_core #403

Conversation

Philipp-M commented Jun 18, 2024 • edited Loading

Philipp-M commented Jun 20, 2024

Philipp-M left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DJMcNab Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Philipp-M commented Jun 27, 2024

DJMcNab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DJMcNab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Philipp-M commented Jun 18, 2024 •

edited

Loading

DJMcNab Jun 24, 2024 •

edited

Loading