nextstrain / nextstrain.org

The Nextstrain website
https://nextstrain.org
GNU Affero General Public License v3.0
87 stars 49 forks source link

[static-site] we're not linting TS files #905

Closed jameshadfield closed 2 weeks ago

jameshadfield commented 3 weeks ago

Our static-site linting rule needs to consider ts and tsx files:

https://github.com/nextstrain/nextstrain.org/blob/5f758195b16654e48e4d3b8057c407af429915ec/package.json#L18

Observed because the nextjs build failed (due to linting errors) in https://github.com/nextstrain/nextstrain.org/pull/904

genehack commented 3 weeks ago

Note: this is also going to require extending/upgrading the ESLint ecosystem to get it to be able to parse Typescript ...

jameshadfield commented 3 weeks ago

Here's how auspice does it - not saying nextstrain.org has to do the same, but if there's no objectively better way then it'd be nice to have consistency.

genehack commented 3 weeks ago

Okay, there's a bit of yak shaving potential here...

Installing @typescript-eslint/eslint-plugin fails due to ...some sort of tangled conflict between peer deps and eslint-config-next (which is a regular dependency, not a devDep, for some reason. We're also installing eslint-plugin-jest but don't seem to be loading/using that anywhere. Finally, we've got a lot of things that are fairly out of date and probably contributing to the kinda high number of audit warnings (currently: 44 vulnerabilities (1 low, 20 moderate, 18 high, 5 critical)):

Package Installed Current
eslint 8.23.1 9.4.0
@babel/eslint-parser 7.19.1 7.24.7
eslint-config-next 14.2.3 14.2.3
eslint-plugin-jest 27.0.4 28.6.0

Due to eslint-config-next lacking ESLint v9 support, we need to stay with ESLint v8, but we could bump to v8.57.0.

I propose to do the following:

~1. remove eslint-plugin-jest~

  1. Update eslint to v8.57.0
  2. install @typescript-eslint/eslint-plugin and @typescript-eslint/parser (I think this will work once the jest plugin is removed, I think it is actually the source of the failing peer dep, because it wants an older version of the typescript parser than eslint-config-next allows for)
  3. update static-site/.eslintrc.yaml to load the TS parser, the TS plugin, and add the @typescript-eslint/recommended ruleset to the extends list
  4. evaluate the linting and see what rules we might want to bring over from nextstrain/auspice (linked above by James)

Potentially a scope expansion, but if we want to push for standardization across nextstrain.org, auspice, and other Nextstrain projects using Typescript, I think the right way to do that is to do the extra work to publish a @nextstrain/nextstrain-eslint-config package on NPM, rather than depending on copypasta to keep files sync'd.

I'm very much on the fence about the value of 100% sync of linting rules across these two projects at this point in time.

I would LOVE to hear opinions and feedback about the above!

tsibley commented 3 weeks ago

I'm very much on the fence about the value of 100% sync of linting rules across these two projects at this point in time.

I would LOVE to hear opinions and feedback about the above!

I don't think our different projects have to have exactly the same linting/setups/whatever. I'm not going to stand in the way of it if folks think it's valuable, but personally I think effort spent to make everything the same in those regards is needless work. I think copying as a starting point is fine, and I think they'll likely diverge over time and that's also fine.

victorlin commented 3 weeks ago

Thanks for catching this, should've been a part of #877!

Figuring out the right versions of packages that work together is a struggle. Your proposal sounds good.

[eslint-config-next] is a regular dependency, not a devDep, for some reason

This is noted in https://github.com/vercel/next.js/issues/50731#issuecomment-1583541139. I think we can manually move it over the devDependencies and things will continue to work fine.

genehack commented 3 weeks ago

Update: eslint-plugin-jest is used, by the ESLint config in the repo root. Plan above updated accordingly.