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

Lint everything #1076

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

Lint everything #1076

wants to merge 4 commits into from

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented Nov 14, 2024

Description of proposed changes

Apply linting rules more uniformly and (TODO) address violations.

Related issue(s)

Prompted by #1073 (comment). I added eqeqeq: error to .eslintrc.yaml, realized it didn't apply to static-site, and went down the rabbit hole.

Checklist

blocked Dependent on external development on Convert static-site from Pages Router to App Router

`npm run build` applies to everything, so the lint script should as
well.
@victorlin victorlin self-assigned this Nov 14, 2024
@nextstrain-bot nextstrain-bot temporarily deployed to nextstrain-s-victorlin--g4efkx November 14, 2024 19:00 Inactive
Copy link
Member Author

Choose a reason for hiding this comment

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

Task: address prefer-const violations

There is 1. I will fix it.

static-site/src/components/ListResources/dodge.js
  50:13  error  'y' is never reassigned. Use 'const' instead  prefer-const

Copy link
Member Author

Choose a reason for hiding this comment

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

Task: address no-use-before-define violations

There are 110. Example:

static-site/src/templates/displayMarkdown.jsx
  52:10  error  'MobileToggleIconContainer' was used before it was defined  no-use-before-define
  53:12  error  'MobileToggleIcon' was used before it was defined           no-use-before-define
  57:10  error  'GreyOverlay' was used before it was defined                no-use-before-define
  72:10  error  'SidebarBodyFlexContainer' was used before it was defined   no-use-before-define
  73:12  error  'SidebarContainer' was used before it was defined           no-use-before-define
  82:12  error  'ContentContainer' was used before it was defined           no-use-before-define
  84:16  error  'PostDate' was used before it was defined                   no-use-before-define
  85:16  error  'PostTitle' was used before it was defined                  no-use-before-define
  86:16  error  'PostAuthor' was used before it was defined                 no-use-before-define

This was added per nextstrain/auspice#1665 (comment). It's categorized as a "Code quality rule" in our ESLint config file, but seems to be more of a stylistic thing. The easiest thing might be to drop the rule, at least for static-site.

Copy link
Member

@jameshadfield jameshadfield Nov 14, 2024

Choose a reason for hiding this comment

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

We should use it but allow function hoisting (see this thread). Most of those errors are from a (formerly? currently?) recommended react style along the lines of:

export const Component = (props) => {
   return (<Container>jsx</Container>)
}
const Container = styled.div``

which looks like is shouldn't work (it works, I think, because Component isn't called by that file, rather it's imported elsewhere so that Container is in scope by the time it runs).

Copy link
Member Author

Choose a reason for hiding this comment

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

I've pushed 95b0ec0 to address this. It covers everything, but not sure if it's the best approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

Task: address operator-linebreak violations

There are 16. Example:

static-site/src/components/sourceInfoHeading.jsx
   96:31  error  '&&' should be placed at the beginning of the line  operator-linebreak
  102:28  error  '&&' should be placed at the beginning of the line  operator-linebreak

This was added per nextstrain/auspice#1665 (comment). It's a code style rule, I'm ambivalent to dropping or addressing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Task: address quote-props violations

There are 7. Example:

static-site/src/components/People/teamMembers.js
    2:3  error  Unnecessarily quoted property 'founders' found  quote-props
   16:3  error  Unnecessarily quoted property 'core' found      quote-props
   85:3  error  Unnecessarily quoted property 'alumni' found    quote-props
  152:3  error  Unnecessarily quoted property 'board' found     quote-props

This was added per nextstrain/auspice#1665 (comment). It's a code style rule, I'm ambivalent to dropping or addressing.

These were intentionally not applied in "static-site: Configure and use
ESLint" (1104d14) by myself, but I don't see a good reason why. It
makes more sense to apply the same baseline rules to all JS/TS code in
this repository, with added React-specific rules for static-site.
@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--g4efkx November 14, 2024 19:17 Inactive
@genehack
Copy link
Contributor

I would say that maybe the time for this is after the App Router conversion is finished, rather than in the middle of it?

@victorlin victorlin added the blocked Dependent on external development label Nov 14, 2024
@victorlin
Copy link
Member Author

@genehack sure, I've updated the PR description and will plan to leave this in draft until that's done.

Reasoning in comments.

For TypeScript files, it's not clear to me why
@typescript-eslint/no-use-before-define defaults to `typedefs: true`:
<https://typescript-eslint.io/rules/no-use-before-define#typedefs>
@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--g4efkx November 15, 2024 00:15 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Dependent on external development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants