Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug in getElementType logic #525

Merged
merged 9 commits into from
May 20, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ module.exports = {
extends: [require.resolve('./lib/configs/recommended'), 'plugin:eslint-plugin/all'],
plugins: ['eslint-plugin'],
rules: {
'import/extensions': 'off',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order for the JS files to be loaded into .mjs, we need an extension.

This library seems to be using a mix of module JS and common JS now, so we'll want to turn this rule off.

'import/no-commonjs': 'off',
'filenames/match-regex': 'off',
'i18n-text/no-en': 'off',
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/nodejs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ jobs:

strategy:
matrix:
node-version: [14, 16, 18]
node-version: [18]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add 20? I've been seeing more Actions migrate to it, and it's now a year old; if nothing breaks might be nice to start testing against it.

(disclaimer: I'm not the expert on node versions.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh okay! let's see what it tells us!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WOO! Added, and passed!


steps:
- uses: actions/checkout@v4
Expand Down
13 changes: 10 additions & 3 deletions lib/utils/get-element-type.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,22 @@ function getElementType(context, node, lazyElementCheck = false) {

// check if the node contains a polymorphic prop
const polymorphicPropName = settings?.github?.polymorphicPropName ?? 'as'

const prop = getProp(node.attributes, polymorphicPropName)
const literalPropValue = getLiteralPropValue(getProp(node.attributes, polymorphicPropName))
let checkConditionalMap = true

// If the prop is not a literal and we cannot determine it, don't fall back to the conditional map value, if it exists
if (prop && !literalPropValue) {
checkConditionalMap = false
}
const rawElement = getLiteralPropValue(getProp(node.attributes, polymorphicPropName)) ?? elementType(node)

// if a component configuration does not exists, return the raw element
if (!settings?.github?.components?.[rawElement]) return rawElement

const defaultComponent = settings.github.components[rawElement]

// check if the default component is also defined in the configuration
return defaultComponent ? defaultComponent : defaultComponent
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this intentional @kendallgassner?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

definitely not

return checkConditionalMap ? settings.github.components[rawElement] : rawElement
}

module.exports = {getElementType}
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
"lint:eslint-docs": "npm run update:eslint-docs -- --check",
"lint:js": "eslint .",
"pretest": "mkdir -p node_modules/ && ln -fs $(pwd) node_modules/",
"test": "npm run eslint-check && npm run lint && mocha tests/**/*.js tests/",
"test": "npm run eslint-check && npm run lint && mocha tests/**/*.js tests/**/*.mjs",
Copy link
Contributor Author

@khiga8 khiga8 May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mjs tests don't actually seem to be running, since this update. Updating this glob seems to fix it.

"update:eslint-docs": "eslint-doc-generator"
},
"repository": {
Expand Down
20 changes: 14 additions & 6 deletions tests/utils/get-element-type.mjs
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
import {expect} from 'chai'
import {getElementType} from '../../lib/utils/get-element-type'
import {mockJSXAttribute, mockJSXConditionalAttribute, mockJSXOpeningElement} from './mocks'

const mocha = require('mocha')
const describe = mocha.describe
const it = mocha.it
import {getElementType} from '../../lib/utils/get-element-type.js'
import {mockJSXAttribute, mockJSXConditionalAttribute, mockJSXOpeningElement} from './mocks.js'
Comment on lines +2 to +3
Copy link
Contributor Author

@khiga8 khiga8 May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently I need to add the .js extension, or I run into failure

Exception during run: Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/home/runner/work/eslint-plugin-github/eslint-plugin-github/lib/utils/get-element-type' imported from /home/runner/work/eslint-plugin-github/eslint-plugin-github/tests/utils/get-element-type.mjs

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This library seems to be using a mix of module JS and common JS now

I wonder if that's why? 🫤

import {describe, it} from 'mocha'

function mockSetting(componentSetting = {}) {
return {
Expand Down Expand Up @@ -63,4 +60,15 @@ describe('getElementType', function () {
])
expect(getElementType({}, node)).to.equal('Box')
})

it('returns raw type when polymorphic prop is set to non-literal expression even with component setting', function () {
// <Box as={isNavigationOpen ? 'generic' : 'navigation'} />
khiga8 marked this conversation as resolved.
Show resolved Hide resolved
khiga8 marked this conversation as resolved.
Show resolved Hide resolved
const setting = mockSetting({
Box: 'div',
})
const node = mockJSXOpeningElement('Box', [
mockJSXConditionalAttribute('as', 'isNavigationOpen', 'generic', 'navigation'),
])
expect(getElementType(setting, node)).to.equal('Box')
})
})
4 changes: 2 additions & 2 deletions tests/utils/get-role.mjs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {expect} from 'chai'
import {getRole} from '../../lib/utils/get-role'
import {mockJSXAttribute, mockJSXConditionalAttribute, mockJSXOpeningElement} from './mocks'
import {getRole} from '../../lib/utils/get-role.js'
import {mockJSXAttribute, mockJSXConditionalAttribute, mockJSXOpeningElement} from './mocks.js'
import {describe, it} from 'mocha'

describe('getRole', function () {
Expand Down
2 changes: 1 addition & 1 deletion tests/utils/object-map.mjs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// @ts-check
import {expect} from 'chai'
import ObjectMap from '../../lib/utils/object-map'
import ObjectMap from '../../lib/utils/object-map.js'
import {describe, it} from 'mocha'

describe('ObjectMap', function () {
Expand Down