Fix ordering issue with keyed virtual nodes

Author: totorigolo

Cargo.toml

@@ -40,9 +40,10 @@ members = [
   "examples/game_of_life",
   "examples/inner_html",
   "examples/js_callback",
+  "examples/keyed_list",
   "examples/large_table",
-  "examples/minimal",
   "examples/minimal_wp",
+  "examples/minimal",
   "examples/mount_point",
   "examples/multi_thread",
   "examples/nested_list",

yew-macro/src/html_tree/html_component.rs

@@ -155,7 +155,7 @@ impl ToTokens for HtmlComponent {
         };
 
         let key = if let Some(key) = props.key() {
-            quote_spanned! { key.span() => Some(#key) }
+            quote_spanned! { key.span() => Some(::yew::virtual_dom::Key::from(#key)) }
         } else {
             quote! {None }
         };

yew-macro/src/html_tree/html_list.rs

@@ -60,7 +60,7 @@ impl ToTokens for HtmlList {
     fn to_tokens(&self, tokens: &mut proc_macro2::TokenStream) {
         let children = &self.children;
         let key = if let Some(key) = &self.key {
-            quote_spanned! {key.span() => Some(#key)}
+            quote_spanned! {key.span() => Some(::yew::virtual_dom::Key::from(#key))}
         } else {
             quote! {None }
         };

yew-macro/src/html_tree/html_tag/mod.rs

@@ -161,7 +161,7 @@ impl ToTokens for HtmlTag {
         });
         let set_key = key.iter().map(|key| {
             quote! {
-                #vtag.key = Some(#key);
+                #vtag.key = Some(::yew::virtual_dom::Key::from(#key));
             }
         });
         let listeners = listeners.iter().map(|listener| {

yew/Cargo.toml

@@ -19,6 +19,7 @@ description = "A framework for making client-side single-page apps"
 travis-ci = { repository = "yewstack/yew" }
 
 [dependencies]
+ahash = { version = "0.3", optional = true }
 anyhow = "1"
 anymap = "0.12"
 bincode = { version = "~1.2.1", optional = true }
@@ -127,6 +128,7 @@ bincode = "~1.2.1"
 
 [features]
 default = ["services", "agent", "web_sys"]
+fast_hasher = ["ahash"]
 std_web = ["stdweb", "yew-macro/std_web"]
 web_sys = ["console_error_panic_hook", "futures", "gloo", "js-sys", "web-sys", "wasm-bindgen", "wasm-bindgen-futures"]
 doc_test = []

yew/src/agent/worker/public.rs

@@ -7,7 +7,6 @@ use cfg_match::cfg_match;
 use slab::Slab;
 use std::any::TypeId;
 use std::cell::RefCell;
-use std::collections::{hash_map, HashMap, HashSet};
 use std::fmt;
 use std::marker::PhantomData;
 use std::rc::Rc;
@@ -21,6 +20,15 @@ cfg_if! {
         use web_sys::{Worker};
     }
 }
+cfg_if! {
+    if #[cfg(feature = "fast_hasher")] {
+        type HashMap<K, V> = ahash::AHashMap<K, V, ahash::RandomState>;
+        type HashSet<K> = ahash::AHashSet<K, ahash::RandomState>;
+        use std::collections::hash_map;
+    } else {
+        use std::collections::{hash_map, HashMap, HashSet};
+    }
+}
 
 thread_local! {
     static REMOTE_AGENTS_POOL: RefCell<AnyMap> = RefCell::new(AnyMap::new());

yew/src/html/scope.rs

@@ -1,6 +1,6 @@
 use super::{Callback, Component, NodeRef, Renderable};
 use crate::scheduler::{scheduler, ComponentRunnableType, Runnable, Shared};
-use crate::virtual_dom::{VDiff, VNode};
+use crate::virtual_dom::{VDiff, VDiffNodePosition, VNode};
 use cfg_if::cfg_if;
 use std::any::{Any, TypeId};
 use std::cell::{Ref, RefCell};
@@ -317,16 +317,20 @@ where
                 state.render_status = state.render_status.map(|_| DIRTY);
                 let mut root = state.component.render();
                 let last_root = state.last_root.take();
-                if let Some(node) =
-                    root.apply(&state.scope.clone().into(), &state.element, None, last_root)
-                {
-                    state.node_ref.set(Some(node));
-                } else if let VNode::VComp(child) = &root {
+                let new_node = root.apply(
+                    &state.scope.clone().into(),
+                    &state.element,
+                    VDiffNodePosition::FirstChild,
+                    last_root,
+                );
+                if let VNode::VComp(child) = &root {
                     // If the root VNode is a VComp, we won't have access to the rendered DOM node
                     // because components render asynchronously. In order to bubble up the DOM node
                     // from the VComp, we need to link the currently rendering component with its
                     // root child component.
                     state.node_ref.link(child.node_ref.clone());
+                } else if let Some(node) = new_node {
+                    state.node_ref.set(Some(node));
                 }
                 state.last_root = Some(root);
             };

yew/src/services/fetch/std_web.rs

@@ -4,8 +4,8 @@ use super::Referrer;
 use crate::callback::Callback;
 use crate::format::{Binary, Format, Text};
 use crate::services::Task;
+use cfg_if::cfg_if;
 use serde::Serialize;
-use std::collections::HashMap;
 use std::fmt;
 use stdweb::serde::Serde;
 use stdweb::unstable::{TryFrom, TryInto};
@@ -15,6 +15,13 @@ use stdweb::{JsSerialize, Value};
 #[allow(unused_imports)]
 use stdweb::{_js_impl, js};
 use thiserror::Error;
+cfg_if! {
+    if #[cfg(feature = "fast_hasher")] {
+        type HashMap<K, V> = ahash::AHashMap<K, V, ahash::RandomState>;
+    } else {
+        use std::collections::HashMap;
+    }
+}
 
 #[doc(no_inline)]
 pub use http::{HeaderMap, Method, Request, Response, StatusCode, Uri};

yew/src/virtual_dom/key.rs

@@ -0,0 +1,30 @@
+//! This module contains the implementation yew's virtual nodes' keys.
+
+use std::rc::Rc;
+
+/// Represents the (optional) key of Yew's virtual nodes.
+///
+/// Keys are cheap to clone.
+// TODO: Explain when keys are useful and add an example.
+#[derive(Clone, Ord, PartialOrd, Eq, PartialEq, Hash)]
+pub struct Key {
+    key: Rc<String>,
+}
+
+impl core::fmt::Display for Key {
+    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+        write!(f, "{}", &self.key)
+    }
+}
+
+impl core::fmt::Debug for Key {
+    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+        write!(f, "{}", &self.key)
+    }
+}
+
+impl<K: Into<Rc<String>>> From<K> for Key {
+    fn from(key: K) -> Self {
+        Key { key: key.into() }
+    }
+}

yew/src/virtual_dom/mod.rs

@@ -1,5 +1,7 @@
 //! This module contains the implementation of reactive virtual dom concept.
 
+#[doc(hidden)]
+pub mod key;
 #[doc(hidden)]
 pub mod vcomp;
 #[doc(hidden)]
@@ -14,19 +16,27 @@ pub mod vtext;
 use crate::html::AnyScope;
 use cfg_if::cfg_if;
 use indexmap::set::IndexSet;
-use std::collections::HashMap;
 use std::fmt;
 use std::rc::Rc;
 cfg_if! {
     if #[cfg(feature = "std_web")] {
         use crate::html::EventListener;
-        use stdweb::web::{Element, Node};
+        use stdweb::web::{Element, INode, Node};
     } else if #[cfg(feature = "web_sys")] {
         use gloo::events::EventListener;
         use web_sys::{Element, Node};
     }
 }
+cfg_if! {
+    if #[cfg(feature = "fast_hasher")] {
+        type HashMap<K, V> = ahash::AHashMap<K, V, ahash::RandomState>;
+    } else {
+        use std::collections::HashMap;
+    }
+}
 
+#[doc(inline)]
+pub use self::key::Key;
 #[doc(inline)]
 pub use self::vcomp::{VChild, VComp};
 #[doc(inline)]
@@ -185,53 +195,106 @@ enum Reform {
 
     /// Create a new reference (js Node).
     ///
-    /// The optional `Node` is used to insert the
-    /// new node in the correct slot of the parent.
+    /// The optional `Node` is used to insert the new node in the correct slot
+    /// of the parent.
     ///
-    /// If it does not exist, a `previous_sibling` must be
-    /// specified (see `VDiff::apply()`).
-    Before(Option<Node>),
+    /// If it does not exist, the `node_position` will be used (see
+    /// `VDiff::apply()`).
+    Replace(VDiffNodePosition),
 }
 
-// TODO(#938): What about to implement `VDiff` for `Element`?
-// In makes possible to include ANY element into the tree.
+#[derive(Debug)]
+pub(crate) enum VDiffNodePosition {
+    FirstChild,
+    Before(Node),
+    After(Node),
+    LastChild,
+}
+
+// TODO(#938): What about implementing `VDiff` for `Element`?
+// It would make it possible to include ANY element into the tree.
 // `Ace` editor embedding for example?
 
 /// This trait provides features to update a tree by calculating a difference against another tree.
 pub(crate) trait VDiff {
-    /// Remove itself from parent and return the next sibling.
-    fn detach(&mut self, parent: &Element) -> Option<Node>;
+    /// Remove self from parent and return the next sibling.
+    fn detach(&mut self, parent: &Element) -> VDiffNodePosition;
 
     /// Scoped diff apply to other tree.
     ///
-    /// Virtual rendering for the node. It uses parent node and existing children (virtual and DOM)
-    /// to check the difference and apply patches to the actual DOM representation.
+    /// Virtual rendering for the node. It uses parent node and existing
+    /// children (virtual and DOM) to check the difference and apply patches to
+    /// the actual DOM representation.
+    ///
+    /// TODO: Explain that vnodes must be at their position before apply is called.
     ///
     /// Parameters:
+    /// - `parent_scope`: the parent `Scope` used for passing messages to the
+    ///   parent `Component`.
     /// - `parent`: the parent node in the DOM.
-    /// - `previous_sibling`: the "previous node" in a list of nodes, used to efficiently
+    /// - `node_position`: the position relative to `parent` used to efficiently
     ///   find where to put the node.
-    /// - `ancestor`: the node that this node will be replacing in the DOM.
-    ///   This method will _always_ remove the `ancestor` from the `parent`.
-    /// - `parent_scope`: the parent `Scope` used for passing messages to the parent `Component`.
+    /// - `ancestor`: the node that this node will be replacing in the DOM. This
+    ///   method will _always_ remove the `ancestor` from the `parent`.
+    ///
+    /// Returns the newly inserted element, if there is one (empty VList don't have one).
     ///
     /// ### Internal Behavior Notice:
     ///
-    /// Note that these modify the DOM by modifying the reference that _already_ exists
-    /// on the `ancestor`. If `self.reference` exists (which it _shouldn't_) this method
-    /// will panic.
+    /// Note that these modify the DOM by modifying the reference that _already_
+    /// exists on the `ancestor`. If `self.reference` exists (which it
+    /// _shouldn't_) this method will panic.
     ///
-    /// The exception to this is obviously `VRef` which simply uses the inner `Node` directly
-    /// (always removes the `Node` that exists).
+    /// The exception to this is obviously `VRef` which simply uses the inner
+    /// `Node` directly (always removes the `Node` that exists).
     fn apply(
         &mut self,
-        scope: &AnyScope,
+        parent_scope: &AnyScope,
         parent: &Element,
-        previous_sibling: Option<&Node>,
+        node_position: VDiffNodePosition, // TODO: merge `node_position` and `ancestor`
         ancestor: Option<VNode>,
     ) -> Option<Node>;
 }
 
+#[cfg(feature = "web_sys")]
+fn insert_node(node: &Node, parent: &Element, node_position: &VDiffNodePosition) -> Node {
+    match node_position {
+        VDiffNodePosition::FirstChild => parent
+            .insert_before(&node, parent.first_child().as_ref())
+            .expect("failed to insert tag before next sibling"),
+        VDiffNodePosition::Before(next_sibling) => parent
+            .insert_before(&node, Some(next_sibling))
+            .expect("failed to insert tag before next sibling"),
+        VDiffNodePosition::After(previous_sibling) => parent
+            .insert_before(&node, previous_sibling.next_sibling().as_ref())
+            .expect("failed to insert tag before next sibling"),
+        VDiffNodePosition::LastChild => parent.append_child(node).expect("failed to append tag"),
+    }
+}
+
+#[cfg(feature = "std_web")]
+fn insert_node(node: &impl INode, parent: &impl INode, node_position: &VDiffNodePosition) -> Node {
+    fn insert_before(node: &impl INode, parent: &impl INode, reference: Option<&Node>) {
+        if let Some(reference) = reference {
+            parent
+                .insert_before(node, reference)
+                .expect("failed to insert tag before next sibling");
+        } else {
+            parent.append_child(node);
+        }
+    }
+
+    match node_position {
+        VDiffNodePosition::FirstChild => insert_before(node, parent, parent.first_child().as_ref()),
+        VDiffNodePosition::Before(next_sibling) => insert_before(node, parent, Some(next_sibling)),
+        VDiffNodePosition::After(previous_sibling) => {
+            insert_before(node, parent, previous_sibling.next_sibling().as_ref())
+        }
+        VDiffNodePosition::LastChild => parent.append_child(node),
+    }
+    node.as_node().clone()
+}
+
 /// Transform properties to the expected type.
 pub trait Transformer<FROM, TO> {
     /// Transforms one type to another.