Migrate Openverse ESLint plugin to FlatConfig #5049

obulat · 2024-10-18T15:40:06Z

Fixes

Description

This PR migrates the Openverse ESLint plugin to the FlatConfig that is used in ESLint version 9.

With this update, we remove many deprecated dependencies, improving security.

Most of the changed lines are replacing let with const (it appears that prefer-const rule was either not enabled or not used before this PR)

Testing Instructions

Try removing some of the eslint-disable comments, and running ov just lint eslint - you should see warnings logged

Apply the following patch to test Openverse-specific rules, and then run ov just lint eslint. You should see warnings for @openverse/eslint-plugin

Openverse rules:

length of a locale string is over 40 words
disabled tests do not have associated github issue
analytics event does not have correct payload

Edit: you can view these errors in the CI run (changes reverted in the most recent commit): https://github.com/WordPress/openverse/actions/runs/11498472780/job/32004338713?pr=5049

Patch to test @openverse/eslint-plugin rules

Subject: [PATCH] Openverse warnings

Index: frontend/test/unit/specs/components/AudioTrack/v-audio-track.spec.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8

diff --git a/frontend/test/unit/specs/components/AudioTrack/v-audio-track.spec.js b/frontend/test/unit/specs/components/AudioTrack/v-audio-track.spec.js
--- a/frontend/test/unit/specs/components/AudioTrack/v-audio-track.spec.js (revision 145e88b)
+++ b/frontend/test/unit/specs/components/AudioTrack/v-audio-track.spec.js (date 1729769395077)
@@ -60,7 +60,7 @@
}
})

it("should render the full audio track component even without duration", async () => {

it.skip("should render the full audio track component even without duration", async () => {
options.props.layout = "full"
const { getByRole } = await render(VAudioTrack, options)
const creator = getByRole("link", { name: props.audio.creator })
Index: packages/js/api-client/tests/client.test.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/packages/js/api-client/tests/client.test.ts b/packages/js/api-client/tests/client.test.ts
--- a/packages/js/api-client/tests/client.test.ts (revision 145e88b)
+++ b/packages/js/api-client/tests/client.test.ts (date 1729769443296)
@@ -21,7 +21,7 @@

describe("OpenverseClient", () => {
describe("api token refresh", async () => {

test("should automatically refresh api token before sending final request", async () => {

test.skip("should automatically refresh api token before sending final request", async () => {
const { client, nock } = getClientAndNock({
clientId: "test",
clientSecret: "test-secret",
Index: frontend/src/types/analytics.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/frontend/src/types/analytics.ts b/frontend/src/types/analytics.ts
--- a/frontend/src/types/analytics.ts (revision 145e88b)
+++ b/frontend/src/types/analytics.ts (date 1729768336706)
@@ -53,6 +53,7 @@

- Questions that are answered by the event must be listed as bullet points.
  */
  export type Events = {

EMPTY_EVENT: {},
/**
- Description: A search performed by the user
- Questions:
  @@ -62,6 +63,7 @@
- - Do most users search from the homepage, or internal searchbar?
    */
    SUBMIT_SEARCH: {
width: number
/** The media type being searched /
searchType: SearchType
/* The search term */
Index: frontend/src/locales/scripts/en.json5
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/frontend/src/locales/scripts/en.json5 b/frontend/src/locales/scripts/en.json5
--- a/frontend/src/locales/scripts/en.json5 (revision 145e88b)
+++ b/frontend/src/locales/scripts/en.json5 (date 1729769241187)
@@ -1,6 +1,6 @@
{
"404": {

title: "The content you’re looking for seems to have disappeared.",

title: "The content you’re looking for seems to have disappeared. The content you’re looking for seems to have disappeared. The content you’re looking for seems to have disappeared. The content you’re looking for seems to have disappeared. for seems to have disappeared.",
main: "Go to {link} or search for something similar from the field below.",
},
hero: {
Index: frontend/test/playwright/e2e/all-results-analytics.spec.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/frontend/test/playwright/e2e/all-results-analytics.spec.ts b/frontend/test/playwright/e2e/all-results-analytics.spec.ts
--- a/frontend/test/playwright/e2e/all-results-analytics.spec.ts (revision 145e88b)
+++ b/frontend/test/playwright/e2e/all-results-analytics.spec.ts (date 1729769395074)
@@ -33,7 +33,7 @@
await goToSearchTerm(page, "birds")
})

test("sends SELECT_SEARCH_RESULT event when audio result is selected", async ({

test.skip("sends SELECT_SEARCH_RESULT event when audio result is selected", async ({
context,
page,
}) => {
Index: packages/js/eslint-plugin/src/rules/analytics-configuration.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/packages/js/eslint-plugin/src/rules/analytics-configuration.ts b/packages/js/eslint-plugin/src/rules/analytics-configuration.ts
--- a/packages/js/eslint-plugin/src/rules/analytics-configuration.ts (revision 145e88b)
+++ b/packages/js/eslint-plugin/src/rules/analytics-configuration.ts (date 1729768071865)
@@ -40,14 +40,8 @@
return true
}

if (
innerTypeAnnotation.type === "TSNullKeyword" ||
innerTypeAnnotation.type === "TSUndefinedKeyword"
) {
return true
}
return false

return innerTypeAnnotation.type === "TSNullKeyword" ||
innerTypeAnnotation.type === "TSUndefinedKeyword";
}

const getPropertyName = (node: TSESTree.TSPropertySignature): string => {

Checklist

My pull request has a descriptive title (not a vague title likeUpdate index.md).
My pull request targets the default branch of the repository (main) or a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.
I ran the DAG documentation generator (ov just catalog/generate-docs for catalog
PRs) or the media properties generator (ov just catalog/generate-docs media-props
for the catalog or ov just api/generate-docs for the API) where applicable.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

github-actions · 2024-10-18T15:58:02Z

Latest k6 run output¹

     ✓ status was 200

     checks.........................: 100.00% ✓ 6400      ✗ 0   
     data_received..................: 1.5 GB  8.6 MB/s
     data_sent......................: 838 kB  4.8 kB/s
     http_req_blocked...............: avg=29.61µs  min=1.99µs  med=3.75µs   max=10.86ms p(90)=5.3µs    p(95)=5.65µs  
     http_req_connecting............: avg=24.83µs  min=0s      med=0s       max=10.79ms p(90)=0s       p(95)=0s      
     http_req_duration..............: avg=605.6ms  min=57.05ms med=522.79ms max=2.33s   p(90)=1.09s    p(95)=1.2s    
       { expected_response:true }...: avg=605.6ms  min=57.05ms med=522.79ms max=2.33s   p(90)=1.09s    p(95)=1.2s    
   ✓ http_req_failed................: 0.00%   ✓ 0         ✗ 6400
     http_req_receiving.............: avg=143.35µs min=41.63µs med=112.61µs max=14.95ms p(90)=177.25µs p(95)=213.36µs
     http_req_sending...............: avg=21.35µs  min=7.06µs  med=19.2µs   max=6.55ms  p(90)=24.96µs  p(95)=27.02µs 
     http_req_tls_handshaking.......: avg=0s       min=0s      med=0s       max=0s      p(90)=0s       p(95)=0s      
     http_req_waiting...............: avg=605.44ms min=56.94ms med=522.59ms max=2.33s   p(90)=1.09s    p(95)=1.2s    
     http_reqs......................: 6400    36.790922/s
     iteration_duration.............: avg=3.23s    min=1.22s   med=2.8s     max=9.49s   p(90)=6.29s    p(95)=6.61s   
     iterations.....................: 1200    6.898298/s
     vus............................: 2       min=2       max=30
     vus_max........................: 30      min=30      max=30

This comment will automatically update with new output each time k6 runs for this PR ↩

sarayourfriend

Cool! I tried a conversion like this a while ago and the flat config was not ready yet... but it has much nicer ergonomics for file inclusion/exclusion! There may be further simplifications possible to the just recipe and pre-commit hook in the future by re-evaluating how files are selected.

sarayourfriend · 2024-10-25T03:14:28Z

package.json

+  "dependencies": {
+    "typescript-eslint": "^8.8.1"
+  },


Can this dependency be a devDependency instead? Or go into the eslint-plugin package? Or does it need to go in the same package.json as typescript?

sarayourfriend · 2024-10-25T03:15:29Z

packages/js/eslint-plugin/src/index.ts

 }
+
+Object.assign(plugin.configs, {


Why use Object.assign here rather than in-lining the configuration into plugin.configs when it's created on line 21?

This is to enable referencing the openverse plugin in the config (to avoid circular imports): https://eslint.org/docs/latest/extend/plugins#configs-in-plugins

krysal · 2024-10-30T18:56:44Z

I got an error when I ran ov just lint eslint. Do you have an idea what might be happening?


.                                        |  WARN  Unsupported engine: wanted: {"node":">= 22.0.0 <23"} (current: {"node":"v20.16.0","pnpm":"9.12.2"})

> @openverse/[email protected] build /Users/krysal/Codes/openverse/packages/js/eslint-plugin
> swc ./src -d ./dist --strip-leading-paths --delete-dir-on-start

Successfully compiled: 11 files with swc (91.42ms)

Oops! Something went wrong! :(

ESLint: 8.57.0

Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'typescript-eslint' imported from /Users/krysal/Codes/openverse/eslint.config.mjs
    at packageResolve (node:internal/modules/esm/resolve:854:9)
    at moduleResolve (node:internal/modules/esm/resolve:927:18)
    at defaultResolve (node:internal/modules/esm/resolve:1169:11)
    at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:383:12)
    at ModuleLoader.resolve (node:internal/modules/esm/loader:352:25)
    at ModuleLoader.getModuleJob (node:internal/modules/esm/loader:227:38)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:87:39)
    at link (node:internal/modules/esm/module_job:86:36)
error: Recipe `eslint` failed with exit code 2

error: Recipe `lint` failed on line 138 with exit code 1

I'm using node v22.11.0.

obulat · 2024-10-30T20:48:44Z

I got an error when I ran ov just lint eslint. Do you have an idea what might be happening?


.                                        |  WARN  Unsupported engine: wanted: {"node":">= 22.0.0 <23"} (current: {"node":"v20.16.0","pnpm":"9.12.2"})

> @openverse/[email protected] build /Users/krysal/Codes/openverse/packages/js/eslint-plugin
> swc ./src -d ./dist --strip-leading-paths --delete-dir-on-start

Successfully compiled: 11 files with swc (91.42ms)

Oops! Something went wrong! :(

ESLint: 8.57.0

Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'typescript-eslint' imported from /Users/krysal/Codes/openverse/eslint.config.mjs
    at packageResolve (node:internal/modules/esm/resolve:854:9)
    at moduleResolve (node:internal/modules/esm/resolve:927:18)
    at defaultResolve (node:internal/modules/esm/resolve:1169:11)
    at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:383:12)
    at ModuleLoader.resolve (node:internal/modules/esm/loader:352:25)
    at ModuleLoader.getModuleJob (node:internal/modules/esm/loader:227:38)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:87:39)
    at link (node:internal/modules/esm/module_job:86:36)
error: Recipe `eslint` failed with exit code 2

error: Recipe `lint` failed on line 138 with exit code 1

I'm using node v22.11.0.

Looks like you need to update your ov environment: it is still using the old node. And the ESLint version logged is old, so you will also need to run ov just node-install after updating the ov node.
I don't remember the commands exactly now, but I think it's ov clean-env && ov init

openverse-bot · 2024-10-31T00:00:10Z

Based on the medium urgency of this PR, the following reviewers are being gently reminded to review this PR:

@krysal
@dhruvkb
This reminder is being automatically generated due to the urgency configuration.

Excluding weekend¹ days, this PR was ready for review 4 day(s) ago. PRs labelled with medium urgency are expected to be reviewed within 4 weekday(s)².

@obulat, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings.

Specifically, Saturday and Sunday. ↩
For the purpose of these reminders we treat Monday - Friday as weekdays. Please note that the operation that generates these reminders runs at midnight UTC on Monday - Friday. This means that depending on your timezone, you may be pinged outside of the expected range. ↩

dhruvkb

I was initially scared by the changes so thanks for the heads up that most of them are let → const. I have commented on some cases of ESLint suppression that I felt can be resolved instead but the PR is good to go.

dhruvkb · 2024-10-31T04:51:38Z

automations/js/src/project_thread_updates.js

+    // eslint-disable-next-line no-constant-binary-expression
    const isDryRun = process.env.DRY_RUN === "true" ?? false


This one caught my eye. Isn't ?? false redundant here? It doesn't seem to have any effect on the value of the expression.

Suggested change

// eslint-disable-next-line no-constant-binary-expression

const isDryRun = process.env.DRY_RUN === "true" ?? false

// eslint-disable-next-line no-constant-binary-expression

const isDryRun = process.env.DRY_RUN === "true" ?? false

dhruvkb · 2024-10-31T04:55:57Z

frontend/tailwind.config.ts

-    require("@tailwindcss/typography"),
+    require("@tailwindcss/typography"), // eslint-disable-line @typescript-eslint/no-require-imports


It is possible to do this without the ESLint disable.

+ import tailwindcssTypography from "@tailwindcss/typography" - require("@tailwindcss/typography"), + tailwindcssTypography,

Signed-off-by: Olga Bulat <[email protected]>

This reverts commit 100ad14.

krysal · 2024-10-31T14:46:06Z

Looks like you need to update your ov environment: it is still using the old node. And the ESLint version logged is old, so you will also need to run ov just node-install after updating the ov node.
I don't remember the commands exactly now, but I think it's ov clean-env && ov init

True, duh! I overlooked that difference. It worked after updating ov :) Thank you, @obulat! Good job here.

obulat force-pushed the update/openverse-eslint-plugin branch from dfe3263 to 7869270 Compare October 18, 2024 15:47

obulat mentioned this pull request Oct 18, 2024

Replace @openverse/eslint-plugin #5006

Closed

1 task

obulat force-pushed the update/openverse-eslint-plugin branch from 55057e1 to 5e81870 Compare October 18, 2024 16:38

obulat self-assigned this Oct 18, 2024

obulat added 🟨 priority: medium Not blocking but should be addressed soon 💻 aspect: code Concerns the software code in the repository 🧱 stack: frontend Related to the Nuxt frontend 🧰 goal: internal improvement Improvement that benefits maintainers, not users labels Oct 18, 2024

openverse-bot added the 🧱 stack: mgmt Related to repo management and automations label Oct 18, 2024

obulat force-pushed the update/openverse-eslint-plugin branch 6 times, most recently from b17ea33 to 26123ca Compare October 24, 2024 08:15

obulat marked this pull request as ready for review October 24, 2024 11:42

obulat requested review from a team as code owners October 24, 2024 11:42

obulat requested review from krysal and dhruvkb and removed request for a team October 24, 2024 11:42

sarayourfriend reviewed Oct 25, 2024

View reviewed changes

obulat force-pushed the update/openverse-eslint-plugin branch 4 times, most recently from c1fe846 to 19ed8f1 Compare October 30, 2024 16:52

obulat force-pushed the update/openverse-eslint-plugin branch from 19ed8f1 to f4a11c6 Compare October 31, 2024 03:53

dhruvkb approved these changes Oct 31, 2024

View reviewed changes

obulat force-pushed the update/openverse-eslint-plugin branch from f4a11c6 to f046c6c Compare October 31, 2024 08:40

obulat added 15 commits October 31, 2024 12:25

Migrate the plugin to ESLint v9

29141ff

Signed-off-by: Olga Bulat <[email protected]>

Fix auto-fixable problems

96baa27

Signed-off-by: Olga Bulat <[email protected]>

Fix other problems

2dfe1ec

Signed-off-by: Olga Bulat <[email protected]>

Update code owners

de1ceb8

Signed-off-by: Olga Bulat <[email protected]>

Use ts config for js files

b190d1d

Signed-off-by: Olga Bulat <[email protected]>

Simplify gitignore handling

ca34490

Signed-off-by: Olga Bulat <[email protected]>

Update plugin playwright

95a189b

Signed-off-by: Olga Bulat <[email protected]>

Re-add vitest eslint rules to tests

e7ce53a

Signed-off-by: Olga Bulat <[email protected]>

Downgrade ESLint to fix typescript-eslint/typescript-eslint#10191

88b12b8

Signed-off-by: Olga Bulat <[email protected]>

Add linting errors

bd15857

Revert "Add linting errors"

017907c

This reverts commit 100ad14.

Update the lock file

30a4111

Post rebase

767b920

Add suggestions from the code review

ef4cdcd

Update dependencies

33cf326

obulat force-pushed the update/openverse-eslint-plugin branch from f046c6c to 33cf326 Compare October 31, 2024 09:49

obulat merged commit 01dfe6f into main Oct 31, 2024
46 checks passed

obulat deleted the update/openverse-eslint-plugin branch October 31, 2024 10:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Openverse ESLint plugin to FlatConfig #5049

Migrate Openverse ESLint plugin to FlatConfig #5049

obulat commented Oct 18, 2024 •

edited

Loading

github-actions bot commented Oct 18, 2024 •

edited

Loading

sarayourfriend left a comment

sarayourfriend Oct 25, 2024

sarayourfriend Oct 25, 2024

obulat Oct 25, 2024

krysal commented Oct 30, 2024

obulat commented Oct 30, 2024

openverse-bot commented Oct 31, 2024

dhruvkb left a comment

dhruvkb Oct 31, 2024

dhruvkb Oct 31, 2024

krysal commented Oct 31, 2024

		// eslint-disable-next-line no-constant-binary-expression
		const isDryRun = process.env.DRY_RUN === "true" ?? false

		require("@tailwindcss/typography"),
		require("@tailwindcss/typography"), // eslint-disable-line @typescript-eslint/no-require-imports

		}

		Object.assign(plugin.configs, {

Migrate Openverse ESLint plugin to FlatConfig #5049

Migrate Openverse ESLint plugin to FlatConfig #5049

Conversation

obulat commented Oct 18, 2024 • edited Loading

Fixes

Description

Testing Instructions

Subject: [PATCH] Openverse warnings

Index: frontend/test/unit/specs/components/AudioTrack/v-audio-track.spec.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8

Checklist

Developer Certificate of Origin

github-actions bot commented Oct 18, 2024 • edited Loading

Latest k6 run output1

Footnotes

sarayourfriend left a comment

Choose a reason for hiding this comment

sarayourfriend Oct 25, 2024

Choose a reason for hiding this comment

sarayourfriend Oct 25, 2024

Choose a reason for hiding this comment

obulat Oct 25, 2024

Choose a reason for hiding this comment

krysal commented Oct 30, 2024

obulat commented Oct 30, 2024

openverse-bot commented Oct 31, 2024

Footnotes

dhruvkb left a comment

Choose a reason for hiding this comment

dhruvkb Oct 31, 2024

Choose a reason for hiding this comment

dhruvkb Oct 31, 2024

Choose a reason for hiding this comment

krysal commented Oct 31, 2024

obulat commented Oct 18, 2024 •

edited

Loading

Index: frontend/test/unit/specs/components/AudioTrack/v-audio-track.spec.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8

github-actions bot commented Oct 18, 2024 •

edited

Loading

Latest k6 run output¹