Add support for props destructure to vue/no-setup-props-reactivity-loss rule (#2550)

ota-meshi · web-flow · commit 2dc606ca589f · 2024-09-18T19:36:15.000+09:00
diff --git a/lib/rules/no-setup-props-reactivity-loss.js b/lib/rules/no-setup-props-reactivity-loss.js
@@ -6,6 +6,60 @@
 const { findVariable } = require('@eslint-community/eslint-utils')
 const utils = require('../utils')
 
+/**
+ * @typedef {'props'|'prop'} PropIdKind
+ *    - `'props'`: A node is a container object that has props.
+ *    - `'prop'`: A node is a variable with one prop.
+ */
+/**
+ * @typedef {object} PropId
+ * @property {Pattern} node
+ * @property {PropIdKind} kind
+ */
+/**
+ * Iterates over Prop identifiers by parsing the given pattern
+ * in the left operand of defineProps().
+ * @param {Pattern} node
+ * @returns {IterableIterator<PropId>}
+ */
+function* iteratePropIds(node) {
+  switch (node.type) {
+    case 'ObjectPattern': {
+      for (const prop of node.properties) {
+        yield prop.type === 'Property'
+          ? {
+              // e.g. `const { prop } = defineProps()`
+              node: unwrapAssignment(prop.value),
+              kind: 'prop'
+            }
+          : {
+              // RestElement
+              // e.g. `const { x, ...prop } = defineProps()`
+              node: unwrapAssignment(prop.argument),
+              kind: 'props'
+            }
+      }
+      break
+    }
+    default: {
+      // e.g. `const props = defineProps()`
+      yield { node: unwrapAssignment(node), kind: 'props' }
+    }
+  }
+}
+
+/**
+ * @template {Pattern} T
+ * @param {T} node
+ * @returns {Pattern}
+ */
+function unwrapAssignment(node) {
+  if (node.type === 'AssignmentPattern') {
+    return node.left
+  }
+  return node
+}
+
 module.exports = {
   meta: {
     type: 'suggestion',
@@ -31,7 +85,9 @@ module.exports = {
   create(context) {
     /**
      * @typedef {object} ScopePropsReferences
-     * @property {Set<Identifier>} refs
+     * @property {object} refs
+     * @property {Set<Identifier>} refs.props A set of references to container objects with multiple props.
+     * @property {Set<Identifier>} refs.prop A set of references a variable with one property.
      * @property {string} scopeName
      */
     /** @type {Map<FunctionDeclaration | FunctionExpression | ArrowFunctionExpression | Program, ScopePropsReferences>} */
@@ -72,70 +128,72 @@ module.exports = {
         wrapperExpressionTypes.has(rightNode.type) &&
         isPropsMemberAccessed(rightNode, propsReferences)
       ) {
-        return report(rightNode, 'getProperty', propsReferences.scopeName)
-      }
-
-      if (
-        left.type !== 'ArrayPattern' &&
-        left.type !== 'ObjectPattern' &&
-        rightNode.type !== 'MemberExpression' &&
-        rightNode.type !== 'ConditionalExpression' &&
-        rightNode.type !== 'TemplateLiteral'
-      ) {
+        // e.g. `const foo = { x: props.x }`
+        report(rightNode, 'getProperty', propsReferences.scopeName)
         return
       }
 
-      if (rightNode.type === 'TemplateLiteral') {
-        rightNode.expressions.some((expression) =>
-          checkMemberAccess(expression, propsReferences, left, right)
-        )
-      } else {
-        checkMemberAccess(rightNode, propsReferences, left, right)
+      // Get the expression that provides the value.
+      /** @type {Expression | Super} */
+      let expression = rightNode
+      while (expression.type === 'MemberExpression') {
+        expression = utils.skipChainExpression(expression.object)
       }
-    }
+      /** A list of expression nodes to verify */
+      const expressions =
+        expression.type === 'TemplateLiteral'
+          ? expression.expressions
+          : expression.type === 'ConditionalExpression'
+            ? [expression.test, expression.consequent, expression.alternate]
+            : expression.type === 'Identifier'
+              ? [expression]
+              : []
 
-    /**
-     * @param {Expression | Super} rightId
-     * @param {ScopePropsReferences} propsReferences
-     * @param {Pattern} left
-     * @param {Expression} right
-     * @return {boolean}
-     */
-    function checkMemberAccess(rightId, propsReferences, left, right) {
-      while (rightId.type === 'MemberExpression') {
-        rightId = utils.skipChainExpression(rightId.object)
-      }
-      if (rightId.type === 'Identifier' && propsReferences.refs.has(rightId)) {
-        report(left, 'getProperty', propsReferences.scopeName)
-        return true
-      }
       if (
-        rightId.type === 'ConditionalExpression' &&
-        (isPropsMemberAccessed(rightId.test, propsReferences) ||
-          isPropsMemberAccessed(rightId.consequent, propsReferences) ||
-          isPropsMemberAccessed(rightId.alternate, propsReferences))
+        (left.type === 'ArrayPattern' || left.type === 'ObjectPattern') &&
+        expressions.some(
+          (expr) =>
+            expr.type === 'Identifier' && propsReferences.refs.props.has(expr)
+        )
       ) {
-        report(right, 'getProperty', propsReferences.scopeName)
-        return true
+        // e.g. `const {foo} = props`
+        report(left, 'getProperty', propsReferences.scopeName)
+        return
+      }
+
+      const reportNode = expressions.find((expr) =>
+        isPropsMemberAccessed(expr, propsReferences)
+      )
+      if (reportNode) {
+        report(reportNode, 'getProperty', propsReferences.scopeName)
       }
-      return false
     }
 
     /**
-     * @param {Expression} node
+     * @param {Expression | Super} node
      * @param {ScopePropsReferences} propsReferences
      */
     function isPropsMemberAccessed(node, propsReferences) {
-      const propRefs = [...propsReferences.refs.values()]
-
-      return propRefs.some((props) => {
+      for (const props of propsReferences.refs.props) {
         const isPropsInExpressionRange = utils.inRange(node.range, props)
         const isPropsMemberExpression =
           props.parent.type === 'MemberExpression' &&
           props.parent.object === props
 
-        return isPropsInExpressionRange && isPropsMemberExpression
-      })
+        if (isPropsInExpressionRange && isPropsMemberExpression) {
+          return true
+        }
+      }
+
+      // Checks for actual member access using prop destructuring.
+      for (const prop of propsReferences.refs.prop) {
+        const isPropsInExpressionRange = utils.inRange(node.range, prop)
+        if (isPropsInExpressionRange) {
+          return true
+        }
+      }
+
+      return false
     }
 
     /**
@@ -149,16 +207,12 @@ module.exports = {
     let scopeStack = null
 
     /**
-     * @param {Pattern | null} node
+     * @param {PropId} propId
      * @param {FunctionDeclaration | FunctionExpression | ArrowFunctionExpression | Program} scopeNode
      * @param {import('eslint').Scope.Scope} currentScope
      * @param {string} scopeName
      */
-    function processPattern(node, scopeNode, currentScope, scopeName) {
-      if (!node) {
-        // no arguments
-        return
-      }
+    function processPropId({ node, kind }, scopeNode, currentScope, scopeName) {
       if (
         node.type === 'RestElement' ||
         node.type === 'AssignmentPattern' ||
@@ -176,7 +230,19 @@ module.exports = {
       if (!variable) {
         return
       }
-      const propsReferenceIds = new Set()
+
+      let scopePropsReferences = setupScopePropsReferenceIds.get(scopeNode)
+      if (!scopePropsReferences) {
+        scopePropsReferences = {
+          refs: {
+            props: new Set(),
+            prop: new Set()
+          },
+          scopeName
+        }
+        setupScopePropsReferenceIds.set(scopeNode, scopePropsReferences)
+      }
+      const propsReferenceIds = scopePropsReferences.refs[kind]
       for (const reference of variable.references) {
         // If reference is in another scope, we can't check it.
         if (reference.from !== currentScope) {
@@ -189,11 +255,8 @@ module.exports = {
 
         propsReferenceIds.add(reference.identifier)
       }
-      setupScopePropsReferenceIds.set(scopeNode, {
-        refs: propsReferenceIds,
-        scopeName
-      })
     }
+
     return utils.compositingVisitors(
       {
         /**
@@ -287,20 +350,29 @@ module.exports = {
           } else if (target.parent.type === 'AssignmentExpression') {
             id = target.parent.right === target ? target.parent.left : null
           }
+          if (!id) return
           const currentScope = utils.getScope(context, node)
-          processPattern(
-            id,
-            context.getSourceCode().ast,
-            currentScope,
-            '<script setup>'
-          )
+          for (const propId of iteratePropIds(id)) {
+            processPropId(
+              propId,
+              context.getSourceCode().ast,
+              currentScope,
+              '<script setup>'
+            )
+          }
         }
       }),
       utils.defineVueVisitor(context, {
         onSetupFunctionEnter(node) {
           const currentScope = utils.getScope(context, node)
           const propsParam = utils.skipDefaultParamValue(node.params[0])
-          processPattern(propsParam, node, currentScope, 'setup()')
+          if (!propsParam) return
+          processPropId(
+            { node: propsParam, kind: 'props' },
+            node,
+            currentScope,
+            'setup()'
+          )
         }
       })
     )
diff --git a/tests/lib/rules/no-setup-props-destructure.js b/tests/lib/rules/no-setup-props-destructure.js
@@ -504,21 +504,6 @@ tester.run('no-setup-props-destructure', rule, {
         }
       ]
     },
-    {
-      filename: 'test.vue',
-      code: `
-      <script setup>
-      const {count} = defineProps({count:Number})
-      </script>
-      `,
-      errors: [
-        {
-          message:
-            'Destructuring the `props` will cause the value to lose reactivity.',
-          line: 3
-        }
-      ]
-    },
     {
       filename: 'test.vue',
       code: `
diff --git a/tests/lib/rules/no-setup-props-reactivity-loss.js b/tests/lib/rules/no-setup-props-reactivity-loss.js
@@ -251,6 +251,22 @@ tester.run('no-setup-props-reactivity-loss', rule, {
       const count = computed(() => props.count)
       </script>
       `
+    },
+    {
+      filename: 'test.vue',
+      code: `
+      <script setup>
+      const {count} = defineProps({count:Number})
+      </script>
+      `
+    },
+    {
+      filename: 'test.vue',
+      code: `
+      <script setup>
+      const { foo = 1, bar = 'ok' } = defineProps({ foo: Number, bar: String })
+      </script>
+      `
     }
   ],
   invalid: [
@@ -532,13 +548,14 @@ tester.run('no-setup-props-reactivity-loss', rule, {
       code: `
       <script setup>
       const {count} = defineProps({count:Number})
+      const foo = count
       </script>
       `,
       errors: [
         {
           message:
-            'Destructuring the `props` will cause the value to lose reactivity.',
-          line: 3
+            'Getting a value from the `props` in root scope of `<script setup>` will cause the value to lose reactivity.',
+          line: 4
         }
       ]
     },
