5a3aa52 · dodrio · git.iko.soy

git.iko.soy/dodrio

on master help sign in

Remove `has_keyed_children(true)` requirement for keying children

5a3aa52Nick Fitzgerald8 years ago

src/builder.rs+25−36

compare with master

unifiedsplit

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

1//! 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};

4use bumpalo::Bump;

6/// A virtual DOM element builder.

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

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

17{

18 bump: &'a Bump,

19− flags: ElementNodeFlags,

19+ key: NodeKey,

20 tag_name: &'a str,

21 listeners: Listeners,

22 attributes: Attributes,

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

62 let bump = bump.into();

63 ElementBuilder {

64 bump,

65− flags: Default::default(),

65+ key: NodeKey::NONE,

66 tag_name,

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

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

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

112 {

113 ElementBuilder {

114 bump: self.bump,

115− flags: self.flags,

115+ key: self.key,

116 tag_name: self.tag_name,

117 listeners,

118 attributes: self.attributes,

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

151 {

152 ElementBuilder {

153 bump: self.bump,

154− flags: self.flags,

154+ key: self.key,

155 tag_name: self.tag_name,

156 listeners: self.listeners,

157 attributes,

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

190 {

191 ElementBuilder {

192 bump: self.bump,

193− flags: self.flags,

193+ key: self.key,

194 tag_name: self.tag_name,

195 listeners: self.listeners,

196 attributes: self.attributes,

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

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

218 ElementBuilder {

219 bump: self.bump,

220− flags: self.flags,

220+ key: self.key,

221 tag_name: self.tag_name,

222 listeners: self.listeners,

223 attributes: self.attributes,

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

226 }

227 }

228

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

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

230− /// configuration is `false`.

231 ///

230 ///

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.

233 ///

235 ///

234− /// ```no_run

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

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

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

238+ /// constraints on the diffing algorithm.

236 ///

239 ///

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

240+ /// # Invariants You Must Uphold

238 ///

241 ///

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

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

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

243+ ///

241− /// .has_keyed_children(true)

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

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`.

258 ///

245 ///

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.

260 ///

248 ///

261 /// # Example

249 /// # Example

262 ///

250 ///

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

271 /// ```

259 /// ```

272 #[inline]

260 #[inline]

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

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

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

262+ use std::u32;

275− self.flags.set_key(key);

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

264+ self.key = NodeKey(key);

276 self

265 self

277 }

266 }

278

267

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

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

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

296 Node::element(

285 Node::element(

297 self.bump,

286 self.bump,

298− self.flags,

287+ self.key,

299 self.tag_name,

288 self.tag_name,

300 self.listeners,

289 self.listeners,

301 self.attributes,

290 self.attributes,

src/cached.rs+8−4

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

1+use crate::{

2+ cached_set::CachedSet,

3+ node::{CachedNode, NodeKey},

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

4+ Node, Render, RenderContext,

5+};

2use std::cell::Cell;

6use std::cell::Cell;

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

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

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

146 cached

150 cached

147 }

151 }

148 _ => {

152 _ => {

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

153+ let mut key = NodeKey::NONE;

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

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

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

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

152− flags = node.flags();

156+ key = node.key();

153 node

157 node

154 });

158 });

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

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

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

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

157 cached

161 cached

158 }

162 }

src/diff.rs+17−9

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

2 cached_set::{CacheId, CachedSet},

3 change_list::ChangeListBuilder,

4 events::EventsRegistry,

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

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

6};

7use fxhash::{FxHashMap, FxHashSet};

8use std::cmp::Ordering;

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

52 (

53 &NodeKind::Element(ElementNode {

54− flags: new_flags,

54+ key: _,

55 tag_name: new_tag_name,

56 listeners: new_listeners,

57 attributes: new_attributes,

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

59 namespace: new_namespace,

60 }),

61 &NodeKind::Element(ElementNode {

62− flags: old_flags,

62+ key: _,

63 tag_name: old_tag_name,

64 listeners: old_listeners,

65 attributes: old_attributes,

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

79 cached_set,

80 change_list,

81 registry,

82− new_flags,

83− old_flags,

84 old_children,

82 old_children,

85 new_children,

83 new_children,

86 cached_roots,

84 cached_roots,

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

212 cached_set: &CachedSet,

210 cached_set: &CachedSet,

213 change_list: &mut ChangeListBuilder,

211 change_list: &mut ChangeListBuilder,

214 registry: &mut EventsRegistry,

212 registry: &mut EventsRegistry,

215− new_flags: ElementNodeFlags,

216− old_flags: ElementNodeFlags,

217 old: &[Node],

213 old: &[Node],

218 new: &[Node],

214 new: &[Node],

219 cached_roots: &mut FxHashSet<CacheId>,

215 cached_roots: &mut FxHashSet<CacheId>,

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

230 return;

226 return;

231 }

227 }

232

228

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 {

234 let t = change_list.next_temporary();

242 let t = change_list.next_temporary();

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

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

236 change_list.set_next_temporary(t);

244 change_list.set_next_temporary(t);

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

877 change_list.create_text_node(text);

885 change_list.create_text_node(text);

878 }

886 }

879 NodeKind::Element(ElementNode {

887 NodeKind::Element(ElementNode {

880− flags: _,

888+ key: _,

881 tag_name,

889 tag_name,

882 listeners,

890 listeners,

883 attributes,

891 attributes,

src/node.rs+27−58

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

41 }

42}

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−

81pub_unstable_internal! {

44pub_unstable_internal! {

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

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

83 /// children.

46 /// children.

84 #[derive(Debug, Clone)]

47 #[derive(Debug, Clone)]

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

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

86− pub flags: ElementNodeFlags,

49+ pub key: NodeKey,

87 pub tag_name: &'a str,

50 pub tag_name: &'a str,

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

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

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

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

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

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

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

101 pub(crate) struct CachedNode {

64 pub(crate) struct CachedNode {

102 pub id: CacheId,

65 pub id: CacheId,

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

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

104− // has flags.

67+ pub key: NodeKey,

105− pub flags: Option<ElementNodeFlags>,

106 }

68 }

107}

69}

108

71+pub_unstable_internal! {

109−/// The key for keyed children.

72+ /// The key for keyed children.

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

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

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

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+}

112

113/// An event listener callback function.

91/// An event listener callback function.

114///

92///

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

188 #[inline]

166 #[inline]

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

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

190 bump: &'a Bump,

168 bump: &'a Bump,

191− flags: ElementNodeFlags,

169+ key: NodeKey,

192 tag_name: &'a str,

170 tag_name: &'a str,

193 listeners: Listeners,

171 listeners: Listeners,

194 attributes: Attributes,

172 attributes: Attributes,

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

211

189

212 Node {

190 Node {

213 kind: NodeKind::Element(ElementNode {

191 kind: NodeKind::Element(ElementNode {

214− flags,

192+ key,

215 tag_name,

193 tag_name,

216 listeners,

194 listeners,

217 attributes,

195 attributes,

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

229 }

207 }

230 }

208 }

231

209

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−

241 #[inline]

210 #[inline]

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

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

243 match &self.kind {

212 match &self.kind {

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

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

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

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

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

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

247 }

216 }

248 }

217 }

249}

218}

src/vdom.rs+2−3

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

2use super::RootRender;

3use crate::cached_set::CachedSet;

4use crate::events::EventsRegistry;

5−use crate::node::Node;

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

6use crate::RenderContext;

7use bumpalo::Bump;

8use futures::future::Future;

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

166

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

168 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);

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

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

172

171

173 let container = container.clone();

172 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>

23where

24 Keys: AsRef<[u16]>,

25{

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

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

28 for &k in keys.as_ref() {

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

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

220 nested_keyed_children {

221 before(cx) {

222 ul(&cx)

223− .has_keyed_children(true)

224 .children([

223 .children([

225 li(&cx)

224 li(&cx)

226 .key(1)

225 .key(1)

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

245 }

244 }

246 after(cx) {

245 after(cx) {

247 ul(&cx)

246 ul(&cx)

248− .has_keyed_children(true)

249 .children([

247 .children([

250 li(&cx)

248 li(&cx)

251 .key(9)

249 .key(9)