Fix bug in `getElementType` logic #525

khiga8 · 2024-05-16T15:51:00Z

This PR fixes a few issues:

1. Fixing bug in `getElementType` logic

const mapping = {
  'Box': 'div'
}

<Box as={something} />

will be interpreted as div. Currently the linter will check the mapping if it is unable to interpret the polymorphic value. However, it should not do that because that is not a correct interpretation.

2. Fixing mjs tests that aren't running

I noticed the tests in utils aren't running even when calling npm run test.

This seems related to this change where the tests were converted to .mjs, but the .mjs tests aren't actually running. When I fixed the command, the tests are being targeted. The test will not run properly without the .js extension.

3. Drops node 14 support

Drops node 14 support per slack thread!

4. Adds node 16 support

khiga8 · 2024-05-16T17:43:39Z

lib/utils/get-element-type.js

-  const defaultComponent = settings.github.components[rawElement]
-
-  // check if the default component is also defined in the configuration
-  return defaultComponent ? defaultComponent : defaultComponent


Was this intentional @kendallgassner?

definitely not

khiga8 · 2024-05-16T19:09:01Z

package.json

@@ -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",


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

khiga8 · 2024-05-16T19:10:49Z

tests/utils/get-element-type.mjs

+import {getElementType} from '../../lib/utils/get-element-type.js'
+import {mockJSXAttribute, mockJSXConditionalAttribute, mockJSXOpeningElement} from './mocks.js'


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

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

I wonder if that's why? 🫤

khiga8 · 2024-05-16T19:15:36Z

@github/web-systems-reviewers

Node 14 builds seem to be failing. Can we drop support?

--

The answer is yes! slack thread!

khiga8 · 2024-05-16T19:41:17Z

.eslintrc.js

@@ -10,6 +10,7 @@ module.exports = {
  extends: [require.resolve('./lib/configs/recommended'), 'plugin:eslint-plugin/all'],
  plugins: ['eslint-plugin'],
  rules: {
+    'import/extensions': 'off',


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.

accessibility-bot · 2024-05-16T20:40:43Z

👋 Hello and thanks for pinging us! You've entered our first responder queue. An accessibility first responder will review this soon.

💻 On PRs for our review: please provide a review environment with steps to validate, screenshots (with alt text), or videos demonstrating functionality we should be checking. This will help speed up our review and feedback cycle.
⚠️ If this is urgent, please visit us in #accessibility on Slack and tag the first responder(s) listed in the channel topic.

JoyceZhu · 2024-05-16T20:43:33Z

.github/workflows/nodejs.yml

@@ -12,7 +12,7 @@ jobs:

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


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.)

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

WOO! Added, and passed!

tests/utils/get-element-type.mjs

dgreif

👍 for web-systems-reviewers

Update tests and command

ca7a3c3

khiga8 commented May 16, 2024

View reviewed changes

Fix bug

9deac20

khiga8 force-pushed the kh-fix-bug-in-get-element-type-logic branch from 87a0e9f to 9deac20 Compare May 16, 2024 17:56

khiga8 added 3 commits May 16, 2024 18:12

Add .js extension so tests pass

f7d1592

fix bugs

9600041

update code comment

e6f3c9e

khiga8 commented May 16, 2024

View reviewed changes

khiga8 marked this pull request as ready for review May 16, 2024 19:11

khiga8 requested a review from a team as a code owner May 16, 2024 19:11

khiga8 requested a review from mattcosta7 May 16, 2024 19:11

khiga8 commented May 16, 2024

View reviewed changes

khiga8 added 2 commits May 16, 2024 20:35

drop node 14

d134206

drop ndoe 16

dd592c6

khiga8 requested review from a team and accessibility-bot May 16, 2024 20:39

JoyceZhu reviewed May 16, 2024

View reviewed changes

add node 20 support

9d2dc5d

JoyceZhu reviewed May 16, 2024

View reviewed changes

tests/utils/get-element-type.mjs Outdated Show resolved Hide resolved

khiga8 commented May 16, 2024

View reviewed changes

tests/utils/get-element-type.mjs Outdated Show resolved Hide resolved

Update tests/utils/get-element-type.mjs

3979a24

JoyceZhu approved these changes May 16, 2024

View reviewed changes

dgreif approved these changes May 16, 2024

View reviewed changes

TylerJDev approved these changes May 17, 2024

View reviewed changes

khiga8 merged commit ce529f9 into main May 20, 2024
4 checks passed

khiga8 deleted the kh-fix-bug-in-get-element-type-logic branch May 20, 2024 17:25

khiga8 mentioned this pull request May 20, 2024

[Bug] Bug in getElementType logic #526

Closed

Sec32fun32 mentioned this pull request Sep 6, 2024

[Snyk] Upgrade eslint-plugin-github from 4.10.2 to 5.0.1 Sec32fun32/checkout#9

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug in `getElementType` logic #525

Fix bug in `getElementType` logic #525

khiga8 commented May 16, 2024 •

edited

Loading

khiga8 May 16, 2024

kendallgassner May 16, 2024

khiga8 May 16, 2024 •

edited

Loading

khiga8 May 16, 2024 •

edited

Loading

JoyceZhu May 16, 2024

khiga8 commented May 16, 2024 •

edited

Loading

khiga8 May 16, 2024

accessibility-bot commented May 16, 2024

JoyceZhu May 16, 2024

khiga8 May 16, 2024

khiga8 May 16, 2024

dgreif left a comment

		import {getElementType} from '../../lib/utils/get-element-type.js'
		import {mockJSXAttribute, mockJSXConditionalAttribute, mockJSXOpeningElement} from './mocks.js'

Fix bug in getElementType logic #525

Fix bug in getElementType logic #525

Conversation

khiga8 commented May 16, 2024 • edited Loading

1. Fixing bug in getElementType logic

2. Fixing mjs tests that aren't running

3. Drops node 14 support

4. Adds node 16 support

khiga8 May 16, 2024

Choose a reason for hiding this comment

kendallgassner May 16, 2024

Choose a reason for hiding this comment

khiga8 May 16, 2024 • edited Loading

Choose a reason for hiding this comment

khiga8 May 16, 2024 • edited Loading

Choose a reason for hiding this comment

JoyceZhu May 16, 2024

Choose a reason for hiding this comment

khiga8 commented May 16, 2024 • edited Loading

khiga8 May 16, 2024

Choose a reason for hiding this comment

accessibility-bot commented May 16, 2024

JoyceZhu May 16, 2024

Choose a reason for hiding this comment

khiga8 May 16, 2024

Choose a reason for hiding this comment

khiga8 May 16, 2024

Choose a reason for hiding this comment

dgreif left a comment

Choose a reason for hiding this comment

Fix bug in `getElementType` logic #525

Fix bug in `getElementType` logic #525

khiga8 commented May 16, 2024 •

edited

Loading

1. Fixing bug in `getElementType` logic

khiga8 May 16, 2024 •

edited

Loading

khiga8 May 16, 2024 •

edited

Loading

khiga8 commented May 16, 2024 •

edited

Loading