5a3aa52 · dodrio · git.iko.soy

git.iko.soy/dodrio

Remove `has_keyed_children(true)` requirement for keying children

commit5a3aa52e81c0b67ce778425faa698ec9e96e6c46 authorNick Fitzgerald <fitzgen@gmail.com> dateWed 17 Apr 2019, 11:43 (8 years ago) parentad5f96e

6 files changed+80−113

compare with master

unifiedsplit

src/builder.rs+25−36 src/cached.rs+8−4 src/diff.rs+17−9 src/node.rs+27−58 src/vdom.rs+2−3 tests/web/keyed.rs+1−3

src/builder.rs+25−36

@@ -1,6 +1,6 @@

11//! Helpers for building virtual DOM nodes.

3−use crate::{node::ElementNodeFlags, Attribute, Listener, Node, RootRender, VdomWeak};

3+use crate::{node::NodeKey, Attribute, Listener, Node, RootRender, VdomWeak};

44use bumpalo::Bump;

66/// A virtual DOM element builder.

@@ -16,7 +16,7 @@ where

1616 Children: 'a + AsRef<[Node<'a>]>,

1717{

1818 bump: &'a Bump,

19− flags: ElementNodeFlags,

19+ key: NodeKey,

2020 tag_name: &'a str,

2121 listeners: Listeners,

2222 attributes: Attributes,

@@ -62,7 +62,7 @@ impl<'a>

6262 let bump = bump.into();

6363 ElementBuilder {

6464 bump,

65− flags: Default::default(),

65+ key: NodeKey::NONE,

6666 tag_name,

6767 listeners: bumpalo::collections::Vec::new_in(bump),

6868 attributes: bumpalo::collections::Vec::new_in(bump),

@@ -112,7 +112,7 @@ where

112112 {

113113 ElementBuilder {

114114 bump: self.bump,

115− flags: self.flags,

115+ key: self.key,

116116 tag_name: self.tag_name,

117117 listeners,

118118 attributes: self.attributes,

@@ -151,7 +151,7 @@ where

151151 {

152152 ElementBuilder {

153153 bump: self.bump,

154− flags: self.flags,

154+ key: self.key,

155155 tag_name: self.tag_name,

156156 listeners: self.listeners,

157157 attributes,

@@ -190,7 +190,7 @@ where

190190 {

191191 ElementBuilder {

192192 bump: self.bump,

193− flags: self.flags,

193+ key: self.key,

194194 tag_name: self.tag_name,

195195 listeners: self.listeners,

196196 attributes: self.attributes,

@@ -217,7 +217,7 @@ where

217217 pub fn namespace(self, namespace: Option<&'a str>) -> Self {

218218 ElementBuilder {

219219 bump: self.bump,

220− flags: self.flags,

220+ key: self.key,

221221 tag_name: self.tag_name,

222222 listeners: self.listeners,

223223 attributes: self.attributes,

@@ -226,37 +226,25 @@ where

226226 }

227227 }

228228

229− /// Set whether this node has keyed children or not. The default

230− /// configuration is `false`.

229+ /// Set this element's key.

231230 ///

232− /// # Example

231+ /// When diffing sets of siblings, if an old sibling and new sibling share a

232+ /// key, then they will always reuse the same physical DOM node. This is

233+ /// important when using CSS animations, web components, third party JS, or

234+ /// anything else that makes the diffing implementation observable.

233235 ///

234− /// ```no_run

235− /// use dodrio::{builder::*, bumpalo::Bump};

236+ /// Do not use keys if such a scenario does not apply. Keyed diffing is

237+ /// generally more expensive than not, since it is putting greater

238+ /// constraints on the diffing algorithm.

236239 ///

237− /// let b = Bump::new();

240+ /// # Invariants You Must Uphold

238241 ///

239− /// // Create a `<ul>` with keyed children.

240− /// let my_ul = ul(&b)

241− /// .has_keyed_children(true)

242− /// .children(create_keyed_children())

243− /// .finish();

244− /// # fn create_keyed_children() -> [dodrio::Node<'static>; 0] { unimplemented!() }

245− /// ```

246− #[inline]

247− pub fn has_keyed_children(mut self, has_keyed_children: bool) -> Self {

248− if has_keyed_children {

249− self.flags |= ElementNodeFlags::HAS_KEYED_CHILDREN;

250− } else {

251− self.flags &= !ElementNodeFlags::HAS_KEYED_CHILDREN;

252− }

253− self

254− }

255−

256− /// Set this element's key. The key must be less than or equal to `2^31 -

257− /// 1`.

242+ /// The key may not be `u32::MAX`, which is a reserved key value.

243+ ///

244+ /// Keys must be unique among siblings.

258245 ///

259− /// This element's parent must have `has_keyed_children(true)` set.

246+ /// All sibling nodes must be keyed, or they must all not be keyed. You may

247+ /// not mix keyed and unkeyed siblings.

260248 ///

261249 /// # Example

262250 ///

@@ -271,8 +259,9 @@ where

271259 /// ```

272260 #[inline]

273261 pub fn key(mut self, key: u32) -> Self {

274− debug_assert_eq!(key & (!ElementNodeFlags::KEY_MASK).bits(), 0);

275− self.flags.set_key(key);

262+ use std::u32;

263+ debug_assert!(key != u32::MAX);

264+ self.key = NodeKey(key);

276265 self

277266 }

278267

@@ -295,7 +284,7 @@ where

295284 pub fn finish(self) -> Node<'a> {

296285 Node::element(

297286 self.bump,

298− self.flags,

287+ self.key,

299288 self.tag_name,

300289 self.listeners,

301290 self.attributes,

src/cached.rs+8−4

@@ -1,4 +1,8 @@

1−use crate::{cached_set::CachedSet, node::CachedNode, Node, Render, RenderContext};

1+use crate::{

2+ cached_set::CachedSet,

3+ node::{CachedNode, NodeKey},

4+ Node, Render, RenderContext,

5+};

26use std::cell::Cell;

37use std::ops::{Deref, DerefMut};

@@ -146,13 +150,13 @@ where

146150 cached

147151 }

148152 _ => {

149− let mut flags = Default::default();

153+ let mut key = NodeKey::NONE;

150154 let id = CachedSet::insert(cx, |nested_cx| {

151155 let node = self.inner.render(nested_cx);

152− flags = node.flags();

156+ key = node.key();

153157 node

154158 });

155− let cached = CachedNode { id, flags };

159+ let cached = CachedNode { id, key };

156160 self.cached.set(Some(cached));

157161 cached

158162 }

src/diff.rs+17−9

@@ -2,7 +2,7 @@ use crate::{

22 cached_set::{CacheId, CachedSet},

33 change_list::ChangeListBuilder,

44 events::EventsRegistry,

5− node::{Attribute, ElementNode, ElementNodeFlags, Listener, Node, NodeKind, TextNode},

5+ node::{Attribute, ElementNode, Listener, Node, NodeKind, TextNode},

66};

77use fxhash::{FxHashMap, FxHashSet};

88use std::cmp::Ordering;

@@ -51,7 +51,7 @@ pub(crate) fn diff(

5151

5252 (

5353 &NodeKind::Element(ElementNode {

54− flags: new_flags,

54+ key: _,

5555 tag_name: new_tag_name,

5656 listeners: new_listeners,

5757 attributes: new_attributes,

@@ -59,7 +59,7 @@ pub(crate) fn diff(

5959 namespace: new_namespace,

6060 }),

6161 &NodeKind::Element(ElementNode {

62− flags: old_flags,

62+ key: _,

6363 tag_name: old_tag_name,

6464 listeners: old_listeners,

6565 attributes: old_attributes,

@@ -79,8 +79,6 @@ pub(crate) fn diff(

7979 cached_set,

8080 change_list,

8181 registry,

82− new_flags,

83− old_flags,

8482 old_children,

8583 new_children,

8684 cached_roots,

@@ -212,8 +210,6 @@ fn diff_children(

212210 cached_set: &CachedSet,

213211 change_list: &mut ChangeListBuilder,

214212 registry: &mut EventsRegistry,

215− new_flags: ElementNodeFlags,

216− old_flags: ElementNodeFlags,

217213 old: &[Node],

218214 new: &[Node],

219215 cached_roots: &mut FxHashSet<CacheId>,

@@ -230,7 +226,19 @@ fn diff_children(

230226 return;

231227 }

232228

233− if new_flags.has_keyed_children() && old_flags.has_keyed_children() {

229+ let new_is_keyed = new[0].key().is_some();

230+ let old_is_keyed = old[0].key().is_some();

231+

232+ debug_assert!(

233+ new.iter().all(|n| n.key().is_some() == new_is_keyed),

234+ "all siblings must be keyed or all siblings must be non-keyed"

235+ );

236+ debug_assert!(

237+ old.iter().all(|o| o.key().is_some() == old_is_keyed),

238+ "all siblings must be keyed or all siblings must be non-keyed"

239+ );

240+

241+ if new_is_keyed && old_is_keyed {

234242 let t = change_list.next_temporary();

235243 diff_keyed_children(cached_set, change_list, registry, old, new, cached_roots);

236244 change_list.set_next_temporary(t);

@@ -877,7 +885,7 @@ fn create(

877885 change_list.create_text_node(text);

878886 }

879887 NodeKind::Element(ElementNode {

880− flags: _,

888+ key: _,

881889 tag_name,

882890 listeners,

883891 attributes,

src/node.rs+27−58

@@ -41,49 +41,12 @@ pub_unstable_internal! {

4141 }

4242}

4343

44−mod flags {

45− bitflags::bitflags! {

46− #[derive(Default)]

47− pub struct ElementNodeFlags: u32 {

48− const HAS_KEYED_CHILDREN = 1 << 31;

49− const KEY_MASK = !(Self::HAS_KEYED_CHILDREN).bits;

50− }

51− }

52−

53− impl ElementNodeFlags {

54− #[inline]

55− pub fn has_keyed_children(self) -> bool {

56− self.contains(ElementNodeFlags::HAS_KEYED_CHILDREN)

57− }

58−

59− #[inline]

60− pub fn key(self) -> u32 {

61− (self & ElementNodeFlags::KEY_MASK).bits

62− }

63−

64− #[inline]

65− pub fn set_key(&mut self, key: u32) {

66− debug_assert_eq!(key & (!ElementNodeFlags::KEY_MASK).bits, 0);

67− self.bits |= key;

68− }

69− }

70−}

71−

72−cfg_if::cfg_if! {

73− if #[cfg(feature = "xxx-unstable-internal-use-only")] {

74− #[doc(hidden)]

75− pub use self::flags::ElementNodeFlags;

76− } else {

77− pub(crate) use self::flags::ElementNodeFlags;

78− }

79−}

80−

8144pub_unstable_internal! {

8245 /// Elements have a tag name, zero or more attributes, and zero or more

8346 /// children.

8447 #[derive(Debug, Clone)]

8548 pub(crate) struct ElementNode<'a> {

86− pub flags: ElementNodeFlags,

49+ pub key: NodeKey,

8750 pub tag_name: &'a str,

8851 pub listeners: &'a [Listener<'a>],

8952 pub attributes: &'a [Attribute<'a>],

@@ -100,15 +63,30 @@ pub_unstable_internal! {

10063 #[derive(Debug, Clone, Copy)]

10164 pub(crate) struct CachedNode {

10265 pub id: CacheId,

103− // Always a copy of the inner cached node's flags when the inner node

104− // has flags.

105− pub flags: Option<ElementNodeFlags>,

66+ // Always a copy of the inner cached node's key.

67+ pub key: NodeKey,

10668 }

10769}

10870

109−/// The key for keyed children.

110−#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]

111−pub(crate) struct NodeKey(u32);

71+pub_unstable_internal! {

72+ /// The key for keyed children.

73+ #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]

74+ pub(crate) struct NodeKey(pub(crate) u32);

75+}

76+

77+impl NodeKey {

78+ pub const NONE: NodeKey = NodeKey(u32::MAX);

79+

80+ #[inline]

81+ pub fn is_none(&self) -> bool {

82+ *self == Self::NONE

83+ }

84+

85+ #[inline]

86+ pub fn is_some(&self) -> bool {

87+ !self.is_none()

88+ }

89+}

11290

11391/// An event listener callback function.

11492///

@@ -188,7 +166,7 @@ impl<'a> Node<'a> {

188166 #[inline]

189167 pub(crate) fn element<Listeners, Attributes, Children>(

190168 bump: &'a Bump,

191− flags: ElementNodeFlags,

169+ key: NodeKey,

192170 tag_name: &'a str,

193171 listeners: Listeners,

194172 attributes: Attributes,

@@ -211,7 +189,7 @@ impl<'a> Node<'a> {

211189

212190 Node {

213191 kind: NodeKind::Element(ElementNode {

214− flags,

192+ key,

215193 tag_name,

216194 listeners,

217195 attributes,

@@ -229,21 +207,12 @@ impl<'a> Node<'a> {

229207 }

230208 }

231209

232− #[inline]

233− pub(crate) fn flags(&self) -> Option<ElementNodeFlags> {

234− match &self.kind {

235− NodeKind::Text(_) => None,

236− NodeKind::Element(e) => Some(e.flags),

237− NodeKind::Cached(c) => c.flags,

238− }

239− }

240−

241210 #[inline]

242211 pub(crate) fn key(&self) -> NodeKey {

243212 match &self.kind {

244− NodeKind::Text(_) => NodeKey(u32::MAX),

245− NodeKind::Element(e) => NodeKey(e.flags.key()),

246− NodeKind::Cached(c) => NodeKey(c.flags.map_or(u32::MAX, |f| f.key())),

213+ NodeKind::Text(_) => NodeKey::NONE,

214+ NodeKind::Element(e) => e.key,

215+ NodeKind::Cached(c) => c.key,

247216 }

248217 }

249218}

src/vdom.rs+2−3

@@ -2,7 +2,7 @@ use super::change_list::ChangeListPersistentState;

22use super::RootRender;

33use crate::cached_set::CachedSet;

44use crate::events::EventsRegistry;

5−use crate::node::Node;

5+use crate::node::{Node, NodeKey};

66use crate::RenderContext;

77use bumpalo::Bump;

88use futures::future::Future;

@@ -166,8 +166,7 @@ impl Vdom {

166166

167167 // Create a dummy `<div/>` in our container.

168168 initialize_container(container);

169− let current_root =

170− Node::element(&dom_buffers[0], Default::default(), "div", [], [], [], None);

169+ let current_root = Node::element(&dom_buffers[0], NodeKey::NONE, "div", [], [], [], None);

171170 let current_root = Some(unsafe { extend_node_lifetime(current_root) });

172171

173172 let container = container.clone();

tests/web/keyed.rs+1−3

@@ -23,7 +23,7 @@ fn keyed<'a, Keys>(cx: &mut RenderContext<'a>, keys: Keys) -> Node<'a>

2323where

2424 Keys: AsRef<[u16]>,

2525{

26− let mut parent = div(&cx).attr("class", "parent").has_keyed_children(true);

26+ let mut parent = div(&cx).attr("class", "parent");

2727

2828 for &k in keys.as_ref() {

2929 parent = parent.child(Keyed(k).render(cx));

@@ -220,7 +220,6 @@ keyed_tests! {

220220 nested_keyed_children {

221221 before(cx) {

222222 ul(&cx)

223− .has_keyed_children(true)

224223 .children([

225224 li(&cx)

226225 .key(1)

@@ -245,7 +244,6 @@ keyed_tests! {

245244 }

246245 after(cx) {

247246 ul(&cx)

248− .has_keyed_children(true)

249247 .children([

250248 li(&cx)

251249 .key(9)