Expressions refactoring + attribute expression value check

src/expression.js

@@ -1,4 +1,4 @@
-import { ATTRIBUTE, EVENT, TEXT } from '@riotjs/util/expression-types'
+import { EVENT, TEXT } from '@riotjs/util/expression-types'
 import expressions from './expressions/index.js'
 import { getTextNode } from './expressions/text.js'
 import { REF_ATTRIBUTE } from './constants.js'
@@ -15,41 +15,41 @@ export const Expression = {
    * @returns {Expression} self
    */
   mount(scope) {
-    // hopefully a pure function
-    this.value = this.evaluate(scope)
-
-    // IO() DOM updates
-    apply(this, this.value)
-
-    return this
+    return this.update(scope)
   },
+
   /**
-   * Update the expression if its value changed
+   * Update the expression
    * @param   {*} scope - argument passed to the expression to evaluate its current values
    * @returns {Expression} self
    */
   update(scope) {
     // pure function
     const value = this.evaluate(scope)
 
-    if (this.value !== value) {
-      // IO() DOM updates
-      apply(this, value)
+    // ref attributes should be called only once during mount
+    if (this.name === REF_ATTRIBUTE && !this.value) {
+      value(this.node)
       this.value = value
+      return this
     }
 
+    // IO() DOM updates
+    apply(this, value)
+    this.value = value
+
     return this
   },
+
   /**
    * Expression teardown method
    * @returns {Expression} self
    */
   unmount() {
     // unmount only the event handling expressions
     if (this.type === EVENT) apply(this, null)
-    // ref attributes need to be unmounted as well
-    if (this.name === REF_ATTRIBUTE)
-      expressions[ATTRIBUTE](null, this, this.value)
+    // ref attributes need to be called with null reference
+    if (this.name === REF_ATTRIBUTE) this.value(null)
 
     return this
   },
@@ -58,16 +58,11 @@ export const Expression = {
 /**
  * IO() function to handle the DOM updates
  * @param {Expression} expression - expression object
- * @param {*} value - current expression value
+ * @param {*} newValue - current expression value
  * @returns {undefined}
  */
-function apply(expression, value) {
-  return expressions[expression.type](
-    expression.node,
-    expression,
-    value,
-    expression.value,
-  )
+function apply(expression, newValue) {
+  return expressions[expression.type](expression, newValue)
 }
 
 export default function create(node, data) {

src/expressions/attribute.js

@@ -4,7 +4,6 @@ import {
   isObject,
 } from '@riotjs/util/checks'
 import { memoize } from '@riotjs/util/misc'
-import { REF_ATTRIBUTE } from '../constants.js'
 
 /* c8 ignore next */
 const ElementProto = typeof Element === 'undefined' ? {} : Element.prototype
@@ -13,89 +12,78 @@ const isNativeHtmlProperty = memoize(
 )
 
 /**
- * Add all the attributes provided
- * @param   {HTMLElement} node - target node
- * @param   {Object} attributes - object containing the attributes names and values
- * @returns {undefined} sorry it's a void function :(
+ * Check whether the attribute value can be rendered
+ * @param {HTMLElement} node - target node
+ * @param {*} value - expression value
+ * @returns {boolean} true if we can render this attribute value
  */
-function setAllAttributes(node, attributes) {
-  Object.keys(attributes).forEach((name) =>
-    attributeExpression(node, { name }, attributes[name]),
+const shouldSetAttribute = (node, value) => {
+  return (
+    ['string', 'number', 'boolean'].includes(typeof value) &&
+    node.getAttribute(name) !== String(value)
   )
 }
 
 /**
- * Remove all the attributes provided
- * @param   {HTMLElement} node - target node
- * @param   {Object} newAttributes - object containing all the new attribute names
- * @param   {Object} oldAttributes - object containing all the old attribute names
- * @returns {undefined} sorry it's a void function :(
+ * Check whether the attribute should be removed
+ * @param {*} value - expression value
+ * @param   {boolean} isBoolean - flag to handle boolean attributes
+ * @returns {boolean} boolean - true if the attribute can be removed
  */
-function removeAllAttributes(node, newAttributes, oldAttributes) {
-  const newKeys = newAttributes ? Object.keys(newAttributes) : []
-
-  Object.keys(oldAttributes)
-    .filter((name) => !newKeys.includes(name))
-    .forEach((attribute) => node.removeAttribute(attribute))
-}
+const shouldRemoveAttribute = (value, isBoolean) =>
+  isBoolean ? !value && value !== 0 : value == null
 
 /**
- * Check whether the attribute value can be rendered
- * @param {*} value - expression value
- * @returns {boolean} true if we can render this attribute value
+ * Get the value as string
+ * @param   {string} name - attribute name
+ * @param   {*} value - user input value
+ * @param   {boolean} isBoolean - boolean attributes flag
+ * @returns {string} input value as string
  */
-function canRenderAttribute(value) {
-  return ['string', 'number', 'boolean'].includes(typeof value)
-}
+const normalizeValue = (name, value, isBoolean) =>
+  // be sure that expressions like selected={ true } will always be rendered as selected='selected'
+  // fix https://github.com/riot/riot/issues/2975
+  value === true && isBoolean ? name : value
 
 /**
- * Check whether the attribute should be removed
- * @param {*} value - expression value
- * @param   {boolean} isBoolean - flag to handle boolean attributes
- * @returns {boolean} boolean - true if the attribute can be removed}
+ * Handle the spread operator {...attributes}
+ * @param   {HTMLElement} node - target node
+ * @param   {Object} value - new expression value
+ * @param   {Object} oldValue - the old expression cached value
+ * @returns {undefined}
  */
-function shouldRemoveAttribute(value, isBoolean) {
-  // boolean attributes should be removed if the value is falsy
-  if (isBoolean) return !value && value !== 0
-  // otherwise we can try to render it
-  return typeof value === 'undefined' || value === null
+function handleSpreadAttributes(node, value, oldValue) {
+  const newKeys = Object.keys(value || [])
+
+  // remove all the old attributes not present in the new values
+  if (oldValue) {
+    Object.keys(oldValue)
+      .filter((name) => !newKeys.includes(name))
+      .forEach((attribute) => node.removeAttribute(attribute))
+  }
+
+  newKeys.forEach((name) => attributeExpression({ node, name }, value[name]))
 }
 
 /**
- * This methods handles the DOM attributes updates
- * @param   {HTMLElement} node - target node
+ * This method handles the DOM attributes updates
  * @param   {Object} expression - expression object
+ * @param   {HTMLElement} expression.node - target node
  * @param   {string} expression.name - attribute name
  * @param   {boolean} expression.isBoolean - flag to handle boolean attributes
+ * @param   {*} expression.value - the old expression cached value
  * @param   {*} value - new expression value
- * @param   {*} oldValue - the old expression cached value
  * @returns {undefined}
  */
 export default function attributeExpression(
-  node,
-  { name, isBoolean },
+  { node, name, isBoolean, value: oldValue },
   value,
-  oldValue,
 ) {
-  // is it a spread operator? {...attributes}
-  if (!name) {
-    if (oldValue) {
-      // remove all the old attributes
-      removeAllAttributes(node, value, oldValue)
-    }
-
-    // is the value still truthy?
-    if (value) {
-      setAllAttributes(node, value)
-    }
-
-    return
-  }
+  if (!node) return
 
-  // ref attributes are treated differently so we early return in this case
-  if (name === REF_ATTRIBUTE) {
-    node && node.removeAttribute(node, name)
-    value(node)
+  // Spread operator {...attributes}
+  if (!name) {
+    handleSpreadAttributes(node, value, oldValue)
     return
   }
 
@@ -107,22 +95,12 @@ export default function attributeExpression(
     node[name] = value
   }
 
+  // remove any attributes with null values or falsy boolean native properties
   if (shouldRemoveAttribute(value, isBoolean)) {
     node.removeAttribute(name)
-  } else if (canRenderAttribute(value)) {
+  }
+  // set new attributes if necessary
+  else if (shouldSetAttribute(node, value)) {
     node.setAttribute(name, normalizeValue(name, value, isBoolean))
   }
 }
-
-/**
- * Get the value as string
- * @param   {string} name - attribute name
- * @param   {*} value - user input value
- * @param   {boolean} isBoolean - boolean attributes flag
- * @returns {string} input value as string
- */
-function normalizeValue(name, value, isBoolean) {
-  // be sure that expressions like selected={ true } will always be rendered as selected='selected'
-  // fix https://github.com/riot/riot/issues/2975
-  return value === true && isBoolean ? name : value
-}

src/expressions/event.js

@@ -19,13 +19,18 @@ const createListener = (node) => {
 
 /**
  * Set a new event listener
- * @param   {HTMLElement} node - target node
  * @param   {Object} expression - expression object
+ * @param   {HTMLElement} expression.node - target node
  * @param   {string} expression.name - event name
  * @param   {*} value - new expression value
  * @returns {value} the callback just received
  */
-export default function eventExpression(node, { name }, value) {
+export default function eventExpression(
+  { node, name, value: oldValue },
+  value,
+) {
+  if (oldValue === value) return
+
   const normalizedEventName = name.replace(RE_EVENTS_PREFIX, '')
   const eventListener = ListenersWeakMap.get(node) || createListener(node)
   const [callback, options] = getCallbackAndOptions(value)

src/expressions/text.js

@@ -12,11 +12,11 @@ export const getTextNode = (node, childNodeIndex) => {
 
 /**
  * This methods handles a simple text expression update
- * @param   {HTMLElement} node - target node
- * @param   {Object} data - expression object
+ * @param   {Object} expression - expression object
+ * @param   {HTMLElement} expression.node - target node
  * @param   {*} value - new expression value
  * @returns {undefined}
  */
-export default function textExpression(node, data, value) {
+export default function textExpression({ node }, value) {
   node.data = normalizeStringValue(value)
 }

src/expressions/value.js

@@ -1,11 +1,13 @@
 import normalizeStringValue from '../util/normalize-string-value.js'
+
 /**
- * This methods handles the input fields value updates
- * @param   {HTMLElement} node - target node
+ * This method handles the input fields value updates
  * @param   {Object} expression - expression object
+ * @param   {HTMLElement} expression.node - target node
+ * @param   {*} expression.value - old expression value
  * @param   {*} value - new expression value
  * @returns {undefined}
  */
-export default function valueExpression(node, expression, value) {
-  node.value = normalizeStringValue(value)
+export default function valueExpression({ node, value: oldValue }, value) {
+  if (oldValue !== value) node.value = normalizeStringValue(value)
 }

test/expressions/attribute.spec.js

@@ -409,4 +409,26 @@ describe('attribute specs', () => {
 
     expect(ref).to.have.been.calledWith(null)
   })
+
+  it('Attributes can check and restore their values upon update (https://github.com/riot/riot/issues/3023)', () => {
+    const target = document.createElement('div')
+    const scope = { attr: { class: 'hello world' } }
+    const el = template('<p expr0></p>', [
+      {
+        selector: '[expr0]',
+        expressions: [
+          { type: expressionTypes.ATTRIBUTE, evaluate: (scope) => scope.attr },
+        ],
+      },
+    ]).mount(target, scope)
+
+    const p = target.querySelector('p')
+
+    expect(p.getAttribute('class')).to.be.equal('hello world')
+    p.classList.remove('world')
+    expect(p.getAttribute('class')).to.be.equal('hello')
+
+    el.update(scope)
+    expect(p.getAttribute('class')).to.be.equal('hello world')
+  })
 })