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

Cleanup: add no-unused-vars and imports eslint rules and fix the codebase #790

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

yannikmesserli
Copy link
Contributor

No issue.

Description

This PR adds the no-unused-vars and imports eslint rules. Then, I ran eslint --fix and finally fixed the remaining issue that couldn't be automatically fixed

Changes

  • Added a new npm package unused-imports which is better than the default rules
  • Enabled the rules in .eslintrc.js with ["error", { "ignoreRestSiblings": true }]
  • Ran eslint --fix
  • Fixed the remaining issues

Screenshots

n/a

Unit tests

They should still all pass

Functional tests

It's probably worth testing quickly the entire app, to see that nothing have been broken.

@yannikmesserli yannikmesserli requested a review from a team as a code owner February 14, 2024 15:01
@yannikmesserli yannikmesserli requested review from kimstacy and removed request for a team February 14, 2024 15:01
@geakstr geakstr requested a review from yandzee February 19, 2024 06:39
@@ -385,6 +385,8 @@ export class Stream<H extends HandlerTypes = {}> extends EventEmitter<Union<Hand
return false;
}

// TODO: fix inheritance - _isFirst is used in Stream implementation so can't be removed here
Copy link
Collaborator

Choose a reason for hiding this comment

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

Feels like its not an issue of inheritance, more like an issue of linter. Would that work if you replaced _isFirst with _ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can _ but then the next person landing on this code will ask himself why there is an unused variable, will likely remove it, and broke again the inheritance. So we should keep the comment to explain why there is an unused variable here until we fix the inheritance, a.k.a we don't add unused variable because parents might use them.

Copy link
Collaborator

@yandzee yandzee left a comment

Choose a reason for hiding this comment

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

I agree with most of this changes, but I don't think that function signatures should be changed. I suggest to try to find an options for linter to disable such behavior.

Its ok to strip unused variables like this:

const fn = () => {
  const thisIsUnused = 'random string';

  return 42;
}

but its not ok to strip function arguments because by this changes you have risks even to break an API (case of messageBuilder args)

@@ -152,7 +151,7 @@ export class Router extends EventEmitter<Handlers> {
this._transaction.push(RouteAction.dropSearchParams());
}

public commit(_commitOpts?: CommitOptions): this {
public commit(): this {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see much advantages on such changes. Feels like there should be a separate rule (or options for existing rules) which could change this point: its wrong to remove unused function args because obviously they can be a part of function signatures.

@yannikmesserli
Copy link
Contributor Author

but I don't think that function signatures should be changed.

Why not? I really don't see a case where an unused parameter of function could be useful. Maybe you can provide an example?

The problem with removing this lint rule is that it's really common for developers to add parameters when they start a new function and not using some of them at the end, after a few massage, while not noticing they're left unused. Think of, for example, a forEach loop: you would think first you'll need the index and ends up without using it. As the code base grows, this will clutter the code, making harder to read it, and therefore, harder to spot bugs.

I suggest that if we find a clear case of a needed unused parameter, we then add a lint exception comment WITH a clear comment to explain why this parameter is absolutely needed, so that no other developer will remove in the future after you.

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.

2 participants