Skip to content

Upgrade eslint and migrate config#4456

Merged
erikgb merged 3 commits intoweaveworks:mainfrom
erikgb:eslint
Jan 8, 2025
Merged

Upgrade eslint and migrate config#4456
erikgb merged 3 commits intoweaveworks:mainfrom
erikgb:eslint

Conversation

@erikgb
Copy link
Copy Markdown
Contributor

@erikgb erikgb commented Jan 6, 2025

Closes #4355 #4418

What changed?

  • Upgrade eslint to the latest release
  • Migrate eslint config to the new flat format (required when upgrading)
  • Fix new lint errors (some adjustments of config instead of trying to fix new issues)

Why was this change made?

We should keep up with releases of tools, as the usually get better.

How was this change implemented?

How did you validate the change?

Release notes

Documentation Changes

@erikgb erikgb changed the title Migrate eslint config Upgrade eslint and migrate config Jan 6, 2025
@erikgb erikgb force-pushed the eslint branch 2 times, most recently from 6f336e3 to 4f10c0a Compare January 7, 2025 07:52
Comment thread eslint.config.mjs Outdated
@mproffitt
Copy link
Copy Markdown
Contributor

Regarding the check errors. All of the switch check errors can be dealt with by the comment I left inline on eslint.config.mjs

For the try {} catch (error) {} error ui/lib/objects.ts - this is a simple fix to convert the catch to simple

try {
...
} catch {

There is no need for the error check at this point as it's simply used to set an empty inventory

That would leave 2 errors

/home/mproffitt/src/github.com/weaveworks/weave-gitops/ui/components/AddKustomizationForm.tsx
  24:7  error  'defaultInitialState' is assigned a value but only used as a type  @typescript-eslint/no-unused-vars

/home/mproffitt/src/github.com/weaveworks/weave-gitops/ui/components/ReconciliationGraph.tsx
  17:1  error  Expected an assignment or function call and instead saw an expression  @typescript-eslint/no-unused-expressions

Dealing with these two in the order shown then for AddKustomizationForm.tsx:

const defaultInitialState = () => ({
  name: "",
  namespace: "",
  source: "",
  path: "",
});

type FormState = ReturnType<typeof defaultInitialState>;

may be better written as simply

type FormState = {
  name: string;
  namespace: string;
  source: string;
  path: string;
};

I'm still not sure how to proceed with the d3 error in ReconciliationGraph.tsx. This becomes a linting error as we move across eslint major versions but I can replicate this error on the enterprise codebase now however I cannot tell the implications of simply removing the statement without extensive additional testing.

I would be inclined to add a per-line disable for this check with a TODO and reason, something like

// TODO: Investigate if this can be removed safely via a component upgrade path
// eslint-disable-next-line @typescript-eslint/no-unused-expressions
d3;

@erikgb
Copy link
Copy Markdown
Contributor Author

erikgb commented Jan 7, 2025

Thanks for the solid review with all the explanations/suggestions for a non-frontend developer, @mproffitt! I will address your suggestions tomorrow!

@erikgb erikgb requested a review from tenstad January 8, 2025 16:58
@erikgb erikgb marked this pull request as ready for review January 8, 2025 16:58
Copy link
Copy Markdown
Contributor

@mproffitt mproffitt left a comment

Choose a reason for hiding this comment

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

Tests currently don't run locally for me but they do run in CI and are successful which is the main thing 🟢

UI still starts and executes in Tilt and a quick visual inspection of the UI shows no apparent changes or breakages have been introduced through fixing lint errors in the code.

image

Overall for a first F/E PR this is fairly solid work.

@erikgb erikgb enabled auto-merge January 8, 2025 18:03
@erikgb erikgb requested a review from casibbald January 8, 2025 18:03
@erikgb
Copy link
Copy Markdown
Contributor Author

erikgb commented Jan 8, 2025

@casibbald or @tenstad, are one you able to approve this PR, so we can get it merged? Or add @mproffitt as an eligible approver? I can probably do the latter, but it feels wrong for me to adjust such settings as a newcomer to this project.

Copy link
Copy Markdown
Collaborator

@casibbald casibbald left a comment

Choose a reason for hiding this comment

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

Fingers crossed

@erikgb erikgb merged commit 3209870 into weaveworks:main Jan 8, 2025
This was referenced Jan 15, 2025
This was referenced Feb 4, 2025
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