-
Notifications
You must be signed in to change notification settings - Fork 49
Bump-Scoped rendering and SVG class attribute fix #136
Changes from 8 commits
22a053f
422bcbf
7489e4e
712c2ec
330045a
a800634
953ed2e
e54519c
788c762
4b01fe7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,8 +21,8 @@ use wasm_bindgen::UnwrapThrowExt; | |
/// | ||
/// pub struct MyComponent; | ||
/// | ||
/// impl Render for MyComponent { | ||
/// fn render<'a>(&self, cx: &mut RenderContext<'a>) -> Node<'a> { | ||
/// impl<'a> Render<'a> for MyComponent { | ||
/// fn render(&self, cx: &mut RenderContext<'a>) -> Node<'a> { | ||
/// use dodrio::builder::*; | ||
/// | ||
/// p(&cx) | ||
|
@@ -35,26 +35,26 @@ use wasm_bindgen::UnwrapThrowExt; | |
/// } | ||
/// } | ||
/// ``` | ||
pub trait Render { | ||
pub trait Render<'a> { | ||
/// Render `self` as a virtual DOM. Use the given context's `Bump` for | ||
/// temporary allocations. | ||
fn render<'a>(&self, cx: &mut RenderContext<'a>) -> Node<'a>; | ||
fn render(&self, cx: &mut RenderContext<'a>) -> Node<'a>; | ||
} | ||
|
||
impl<'r, R> Render for &'r R | ||
impl<'a, 'r, R> Render<'a> for &'r R | ||
where | ||
R: Render, | ||
R: Render<'a>, | ||
{ | ||
fn render<'a>(&self, cx: &mut RenderContext<'a>) -> Node<'a> { | ||
fn render(&self, cx: &mut RenderContext<'a>) -> Node<'a> { | ||
(**self).render(cx) | ||
} | ||
} | ||
|
||
impl<R> Render for Rc<R> | ||
impl<'a, R> Render<'a> for Rc<R> | ||
where | ||
R: Render, | ||
R: Render<'a>, | ||
{ | ||
fn render<'a>(&self, cx: &mut RenderContext<'a>) -> Node<'a> { | ||
fn render(&self, cx: &mut RenderContext<'a>) -> Node<'a> { | ||
(**self).render(cx) | ||
} | ||
} | ||
|
@@ -69,7 +69,7 @@ where | |
/// You do not need to implement this trait by hand: there is a blanket | ||
/// implementation for all `Render` types that fulfill the `RootRender` | ||
/// requirements. | ||
pub trait RootRender: Any + Render { | ||
pub trait RootRender: Any + for<'a> Render<'a> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In #46 @fitzgen mentioned this is the same as The way I understand it, this trait bound says RootRender requires its types to also implement Any and Render for all lifetimes Am I missing something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If something is valid for all lifetimes then it must be The only time that For this case, I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I think we both might be right. Even though Render is a generic trait, it is object safe when used with higher-order lifetimes. That indicates to me that you are correct about That being said, the bound use dodrio::{bumpalo::{self, Bump}};
struct Context<'a> {
bump: &'a Bump
}
#[derive(Debug, PartialEq, Eq)]
struct Node<'a> {
name: &'a str
}
trait Render<'a> {
fn render(&self, cx: &mut Context<'a>) -> Node<'a>;
}
struct Test;
impl<'a> Render<'a> for Test {
fn render(&self, cx: &mut Context<'a>) -> Node<'a> {
Node { name: bumpalo::format!(in cx.bump, "Test{}", 1).into_bump_str() }
}
}
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn it_works() {
let bump = Bump::default();
let mut cx = Context { bump: &bump };
let comp: Box<dyn for<'a> Render<'a>> = Box::new(Test);
assert_eq!(Node { name: "Test1" }, comp.render(&mut cx));
}
} This test compiles and runs as expected. It's emulating the situation I currently have in my fork of dodrio. Now, when I switch the type of comp to use dodrio::{bumpalo::{self, Bump}};
struct Context<'a> {
bump: &'a Bump
}
#[derive(Debug, PartialEq, Eq)]
struct Node<'a> {
name: &'a str
}
trait Render<'a> {
fn render(&self, cx: &mut Context<'a>) -> Node<'a>;
}
struct Test;
impl<'a> Render<'a> for Test {
fn render(&self, cx: &mut Context<'a>) -> Node<'a> {
Node { name: bumpalo::format!(in cx.bump, "Test{}", 1).into_bump_str() }
}
}
#[cfg(test)]
mod tests {
use super::*;
#[test]
fn it_works() {
let bump = Bump::default();
let mut cx = Context { bump: &bump };
let comp: Box<dyn Render<'static> + 'static> = Box::new(Test);
assert_eq!(Node { name: "Test1" }, comp.render(&mut cx));
}
} Compiler output:
This is because while the If I'm missing something, could you show me what you mean with this example? Maybe we're talking about different things. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah ok, this makes sense. Thanks for breaking it down! |
||
/// Get this `&RootRender` trait object as an `&Any` trait object reference. | ||
fn as_any(&self) -> &dyn Any; | ||
|
||
|
@@ -80,7 +80,7 @@ pub trait RootRender: Any + Render { | |
|
||
impl<T> RootRender for T | ||
where | ||
T: Any + Render, | ||
T: Any + for<'a> Render<'a>, | ||
{ | ||
fn as_any(&self) -> &dyn Any { | ||
self | ||
|
@@ -138,4 +138,31 @@ mod tests { | |
#[allow(dead_code)] | ||
fn takes_dyn_render(_: &dyn super::RootRender) {} | ||
} | ||
|
||
#[test] | ||
fn render_bump_scoped_child() { | ||
use crate::{builder::*, bumpalo::collections::String, Node, Render, RenderContext}; | ||
|
||
struct Child<'a> { | ||
name: &'a str, | ||
} | ||
|
||
impl<'a> Render<'a> for Child<'a> { | ||
fn render(&self, _cx: &mut RenderContext<'a>) -> Node<'a> { | ||
text(self.name) | ||
} | ||
} | ||
|
||
struct Parent; | ||
|
||
impl<'a> Render<'a> for Parent { | ||
fn render(&self, cx: &mut RenderContext<'a>) -> Node<'a> { | ||
let child_name = String::from_str_in("child", cx.bump).into_bump_str(); | ||
|
||
div(&cx) | ||
.children([Child { name: child_name }.render(cx)]) | ||
.finish() | ||
} | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should act as a smoke test for bump-scoped lifetimes in child components There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I should have been more clear. This does ensure that code with bump-scoped child components compiles, but I was hoping for a rendering test that ensures that we also get the expected physical dom. Should be fine to put it in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, ok. I'll get that taken care of sometime tonight. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
use super::{assert_rendered, before_after, create_element, RenderFn}; | ||
use dodrio::{builder::*, Vdom}; | ||
use std::rc::Rc; | ||
use wasm_bindgen::{ JsCast}; | ||
use wasm_bindgen_test::*; | ||
|
||
#[wasm_bindgen_test] | ||
|
@@ -34,6 +35,58 @@ fn container_is_emptied_upon_drop() { | |
assert!(container.first_child().is_none()); | ||
} | ||
|
||
/// Originally, dodrio would use go through the className property for SVGs. | ||
/// | ||
/// This is problematic because when SVG elements are created, the className is flagged as a read | ||
/// only property, so setting it causes an exception to be thrown. Here's an example of how this | ||
/// happens: | ||
/// | ||
/// let elem = web_sys::window() | ||
/// .unwrap() | ||
/// .document() | ||
/// .unwrap() | ||
/// .create_element_ns(Some("http://www.w3.org/2000/svg"), "svg") | ||
/// .unwrap(); | ||
/// | ||
/// elem.set_class_name("does-not-work"); | ||
/// | ||
/// ----------------------------------------------------------------------------------------------- | ||
/// | ||
/// wasm-bindgen: imported JS function that was not marked as `catch` threw an error: | ||
/// setting getter-only property "className" | ||
/// | ||
/// ----------------------------------------------------------------------------------------------- | ||
/// | ||
/// Now, dodrio passes the 'class' attribute of all namespaced elements into set_attribute. This | ||
/// satisfies the restrictions on SVG and keeps the optimized path for non-namespaced elements | ||
#[wasm_bindgen_test(async)] | ||
async fn test_svg_set_class() { | ||
let container = create_element("div"); | ||
|
||
|
||
let valid_svg = Rc::new(RenderFn(|cx| { | ||
ElementBuilder::new(cx.bump, "svg") | ||
.namespace(Some("http://www.w3.org/2000/svg")) | ||
.attr("class", "works") | ||
.finish() | ||
})); | ||
|
||
let vdom = Vdom::new(&container, valid_svg.clone()); | ||
let weak = vdom.weak(); | ||
|
||
weak.render().await.unwrap(); | ||
|
||
assert_eq!( | ||
"works", | ||
container.first_child() | ||
.expect("unable to get svg") | ||
.dyn_ref::<web_sys::Element>() | ||
.expect("svg should be an element") | ||
.get_attribute("class") | ||
.expect("unable to get 'class' of svg") | ||
); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's the regression test. I wanted to test the failure case too, but web_sys doesn't mark You can verify that this works by running the following:
Then, verify that the test fails when the new change is disabled: // src/change_list/mod.rs - comment out !is_namespaced
pub fn set_attribute(&mut self, name: &str, value: &str, is_namespaced: bool) {
debug_assert!(self.traversal_is_committed());
if name == "class" /* && !is_namespaced*/ {
let class_id = self.ensure_string(value);
debug!("emit: set_class({:?})", value);
self.state.emitter.set_class(class_id.into());
} else {
let name_id = self.ensure_string(name);
let value_id = self.ensure_string(value);
debug!("emit: set_attribute({:?}, {:?})", name, value);
self.state
.emitter
.set_attribute(name_id.into(), value_id.into());
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BTW, I wasn't sure where to put the test, so let me know if you want to move it somewhere else. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a fine location -- thanks! |
||
before_after! { | ||
same_text { | ||
before(_cx) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's where the SVG change begins and ends. I believe it's just this one if statement.