Merge pull request #461 from github/kh-fix-bug-with-methods

Fix bugs with `getRole` and `getElementType`

lib/utils/get-element-type.js

@@ -1,4 +1,4 @@
-const {elementType, getProp, getPropValue} = require('jsx-ast-utils')
+const {elementType, getProp, getLiteralPropValue} = require('jsx-ast-utils')
 
 /*
 Allows custom component to be mapped to an element type.
@@ -12,7 +12,7 @@ function getElementType(context, node, ignoreMap = false) {
 
   // check if the node contains a polymorphic prop
   const polymorphicPropName = settings?.github?.polymorphicPropName ?? 'as'
-  const rawElement = getPropValue(getProp(node.attributes, polymorphicPropName)) ?? elementType(node)
+  const rawElement = getLiteralPropValue(getProp(node.attributes, polymorphicPropName)) ?? elementType(node)
 
   // if a component configuration does not exists, return the raw element
   if (ignoreMap || !settings?.github?.components?.[rawElement]) return rawElement

lib/utils/get-role.js

@@ -1,4 +1,4 @@
-const {getProp, getPropValue} = require('jsx-ast-utils')
+const {getProp, getLiteralPropValue} = require('jsx-ast-utils')
 const {elementRoles} = require('aria-query')
 const {getElementType} = require('./get-element-type')
 const ObjectMap = require('./object-map')
@@ -43,7 +43,7 @@ function cleanElementRolesMap() {
 */
 function getRole(context, node) {
   // Early return if role is explicitly set
-  const explicitRole = getPropValue(getProp(node.attributes, 'role'))
+  const explicitRole = getLiteralPropValue(getProp(node.attributes, 'role'))
   if (explicitRole) {
     return explicitRole
   }
@@ -80,7 +80,7 @@ function getRole(context, node) {
       continue
     }
 
-    const value = getPropValue(propOnNode)
+    const value = getLiteralPropValue(propOnNode)
     if (value || (value === '' && prop === 'alt')) {
       if (
         prop === 'href' ||

tests/a11y-role-supports-aria-props.js

@@ -36,6 +36,9 @@ ruleTester.run('a11y-role-supports-aria-props', rule, {
     {code: '<div role="presentation" {...props} />'},
     {code: '<Foo.Bar baz={true} />'},
     {code: '<Link href="#" aria-checked />'},
+    // Don't try to evaluate expression
+    {code: '<Box aria-labelledby="some-id" role={role} />'},
+    {code: '<Box aria-labelledby="some-id"as={isNavigationOpen ? "div" : "nav"} />'},
 
     // IMPLICIT ROLE TESTS
     // A TESTS - implicit role is `link`
@@ -479,12 +482,17 @@ ruleTester.run('a11y-role-supports-aria-props', rule, {
       errors: [getErrorMessage('aria-labelledby', 'generic')],
     },
     {
-      code: '<div aria-label />',
-      errors: [getErrorMessage('aria-label', 'generic')],
+      code: '<div aria-labelledby />',
+      errors: [getErrorMessage('aria-labelledby', 'generic')],
     },
+    // Determines role from literal `as` prop.
     {
-      code: '<div aria-labelledby />',
+      code: '<Box as="span" aria-labelledby />',
       errors: [getErrorMessage('aria-labelledby', 'generic')],
     },
+    {
+      code: '<p role="generic" aria-label />',
+      errors: [getErrorMessage('aria-label', 'generic')],
+    },
   ],
 })

tests/utils/get-element-type.js

@@ -1,5 +1,5 @@
 const {getElementType} = require('../../lib/utils/get-element-type')
-const {mockJSXAttribute, mockJSXOpeningElement} = require('./mocks')
+const {mockJSXAttribute, mockJSXConditionalAttribute, mockJSXOpeningElement} = require('./mocks')
 
 const mocha = require('mocha')
 const describe = mocha.describe
@@ -55,4 +55,12 @@ describe('getElementType', function () {
     const node = mockJSXOpeningElement('Link', [mockJSXAttribute('as', 'Button')])
     expect(getElementType(setting, node)).to.equal('button')
   })
+
+  it('returns raw type when polymorphic prop is set to non-literal expression', function () {
+    // <Box as={isNavigationOpen ? 'generic' : 'navigation'} />
+    const node = mockJSXOpeningElement('Box', [
+      mockJSXConditionalAttribute('as', 'isNavigationOpen', 'generic', 'navigation'),
+    ])
+    expect(getElementType({}, node)).to.equal('Box')
+  })
 })

tests/utils/get-role.js

@@ -1,11 +1,31 @@
 const {getRole} = require('../../lib/utils/get-role')
-const {mockJSXAttribute, mockJSXOpeningElement} = require('./mocks')
+const {mockJSXAttribute, mockJSXConditionalAttribute, mockJSXOpeningElement} = require('./mocks')
 const mocha = require('mocha')
 const describe = mocha.describe
 const it = mocha.it
 const expect = require('chai').expect
 
 describe('getRole', function () {
+  it('returns undefined when polymorphic prop is set with a non-literal expression', function () {
+    // <Box as={isNavigationOpen ? 'div' : 'nav'} />
+    const node = mockJSXOpeningElement('Box', [mockJSXConditionalAttribute('as', 'isNavigationOpen', 'div', 'nav')])
+    expect(getRole({}, node)).to.equal(undefined)
+  })
+
+  it('returns undefined when role is set to non-literal expression', function () {
+    // <Box role={isNavigationOpen ? 'generic' : 'navigation'} />
+    const node = mockJSXOpeningElement('Box', [
+      mockJSXConditionalAttribute('role', 'isNavigationOpen', 'generic', 'navigation'),
+    ])
+    expect(getRole({}, node)).to.equal(undefined)
+  })
+
+  it('returns `role` when set to a literal expression', function () {
+    // <Box role="generic" />
+    const node = mockJSXOpeningElement('Box', [mockJSXAttribute('role', 'generic')])
+    expect(getRole({}, node)).to.equal('generic')
+  })
+
   it('returns generic role for <span> regardless of attribute', function () {
     const node = mockJSXOpeningElement('span', [mockJSXAttribute('aria-label', 'something')])
     expect(getRole({}, node)).to.equal('generic')

tests/utils/mocks.js

@@ -12,6 +12,38 @@ function mockJSXAttribute(prop, propValue) {
   }
 }
 
+/* Mocks conditional expression
+  e.g. <Box as={isNavigationOpen ? 'generic' : 'navigation'} /> can be by calling
+  mockJSXConditionalAttribute('as', 'isNavigationOpen', 'generic', 'navigation')
+*/
+function mockJSXConditionalAttribute(prop, expression, consequentValue, alternateValue) {
+  return {
+    type: 'JSXAttribute',
+    name: {
+      type: 'JSXIdentifier',
+      name: prop,
+    },
+    value: {
+      type: 'JSXExpressionContainer',
+      value: prop,
+      expression: {
+        type: 'ConditionalExpression',
+        test: {
+          type: expression,
+        },
+        consequent: {
+          type: 'Literal',
+          value: consequentValue,
+        },
+        alternate: {
+          type: 'Literal',
+          value: alternateValue,
+        },
+      },
+    },
+  }
+}
+
 function mockJSXOpeningElement(tagName, attributes = []) {
   return {
     type: 'JSXOpeningElement',
@@ -23,4 +55,4 @@ function mockJSXOpeningElement(tagName, attributes = []) {
   }
 }
 
-module.exports = {mockJSXAttribute, mockJSXOpeningElement}
+module.exports = {mockJSXAttribute, mockJSXOpeningElement, mockJSXConditionalAttribute}