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

Update to ESlint 9 and flat config #4358

Merged
merged 2 commits into from
Jan 7, 2025
Merged

Update to ESlint 9 and flat config #4358

merged 2 commits into from
Jan 7, 2025

Conversation

rakyi
Copy link
Contributor

@rakyi rakyi commented Dec 26, 2024

  • Run npx react-codemod update-react-imports and fix the no longer necessary hack to suppress warning about useLayoutEffect when server side rendering, see the commit for more context
  • Update ESlint and related packages
  • Migrate the base ESlint config to the new flat format
  • Delete all nested .eslintrc.yaml config files, since nested dir configs are no longer supported, and move the same config for those directories into the main eslint.config.mjs file
  • Fix all lint errors and warnings

I tried switching to the new projectService config option for typescript-eslint, and it was much slower than the old project option, so I kept the old one.

@owidbot
Copy link
Contributor

owidbot commented Dec 26, 2024

Quick links (staging server):

Site Dev Site Preview Admin Wizard Docs

Login: ssh owid@staging-site-eslint-9

SVG tester:

Number of differences (default views): 0 ✅
Number of differences (all views): 0 ✅

Edited: 2024-12-26 15:27:21 UTC
Execution time: 1.19 seconds

Copy link
Contributor

@danyx23 danyx23 left a comment

Choose a reason for hiding this comment

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

Very nice, thanks! I had a few questions but more for clarification for me - feel free to merge!

@@ -20,7 +20,7 @@ export const RESEARCH_AND_WRITING_DEFAULT_HEADING = "Research & Writing"
* https://docs.google.com/spreadsheets/d/abcd1234
*/
export const gdocUrlRegex =
/https:\/\/docs\.google\.com\/document(?:\/u\/\d)?\/d\/([\-\w]+)\/?(edit)?(\?tab=[\w\.]+)?#?/
/https:\/\/docs\.google\.com\/document(?:\/u\/\d)?\/d\/([-\w]+)\/?(edit)?(\?tab=[\w.]+)?#?/
Copy link
Contributor

Choose a reason for hiding this comment

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

Most of the regex edits in this PR are fine as is but this one is semantically differen, no? \. is not the same as . - or am I missing something? It doesn't matter much in this case, but just for consistency :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ESlint is convinced that escaping . in brackets is unnecessary, and it seems to be correct:

/[\.]/.test("abc")
false
/[.]/.test("abc")
false
/\./.test("abc")
false
/./.test("abc")
true

@@ -436,14 +436,15 @@ export class StackedDiscreteBarChart
case SortBy.entityName:
sortByFunc = (item: Item): string => item.entityName
break
case SortBy.column:
case SortBy.column: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason why you added these braces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because of the no-case-declarations rule.

@@ -128,9 +128,9 @@ function extractSchemaRecursive(
// then do not emit anything directly and recurse over the
// described properties
if (
schema.hasOwnProperty("type") &&
Object.prototype.hasOwnProperty.call(schema, "type") &&
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because of the no-prototype-builtins rule.

Copy link
Contributor Author

rakyi commented Jan 3, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link
Contributor Author

rakyi commented Jan 7, 2025

Merge activity

  • Jan 7, 3:47 AM EST: A user started a stack merge that includes this pull request via Graphite.
  • Jan 7, 3:48 AM EST: Graphite rebased this pull request as part of a merge.
  • Jan 7, 3:49 AM EST: A user merged this pull request with Graphite.

rakyi added 2 commits January 7, 2025 08:48
Since React 17 we no longer need to import React for JSX transforms.
Running this codemod:

* Removes all unused React imports as a result of upgrading to the new
  JSX transform.
* Changes all default React imports (i.e. import React from "react") to
  destructured named imports (ex. import { useState } from "react") which
  is the preferred style going into the future.

These cannot be separated, even though we now only needed to remove the
unused imports.
@rakyi rakyi merged commit 68b3d81 into master Jan 7, 2025
12 of 16 checks passed
@rakyi rakyi deleted the eslint-9 branch January 7, 2025 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants