Skip to content

Upgrade eslint and migrate config #4456

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

Merged
merged 3 commits into from
Jan 8, 2025
Merged

Upgrade eslint and migrate config #4456

merged 3 commits into from
Jan 8, 2025

Conversation

erikgb
Copy link
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
@mproffitt
Copy link
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
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
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
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
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
22 checks passed
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