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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .eslintrc.yaml
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

Choose a reason for hiding this comment

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

We shouldn't be enforcing this in my opinion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. Let's drop the rule entirely from nextstrain.org and auspice?

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.

Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ ignorePatterns:
- auspice-client/
- docs/
- node_modules/
- static-site/

rules:
# Code quality rules
eqeqeq: error
no-unused-vars: error
no-use-before-define: ["error", { "functions": false, "classes": false }]
prefer-const: ["error", {"destructuring": "all"}]
Expand Down
3 changes: 1 addition & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,7 @@ jobs:
node-version-file: 'package.json'
- run: node --version
- run: npm ci
- run: npm run lint:server
- run: npm run lint:static-site
- run: npm run lint
- run: node ./scripts/check-resource-index-match.js

# Build into Heroku slug so we can deploy the same build that we test.
Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@
"scripts": {
"build": "./build.sh",
"build:feeds": "node scripts/generate-blog-feeds.js",
"lint": "npm run lint:server",
"lint:server": "DEBUG=eslint:cli-engine npx eslint --max-warnings=0 --ext .js,.jsx .",
"lint": "DEBUG=eslint:cli-engine npx eslint --max-warnings=0 --ext .js,.jsx,.ts,.tsx .",
"lint:server": "DEBUG=eslint:cli-engine npx eslint --max-warnings=0 --ext .js,.jsx --ignore-pattern static-site .",
"lint:static-site": "cd static-site && DEBUG=eslint:cli-engine npx eslint --max-warnings=0 --ext .js,.jsx,.ts,.tsx .",
"server": "node server.js",
"groups": "node server.js --app ./src/groupsApp.js",
Expand Down
4 changes: 2 additions & 2 deletions src/authz/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ import tags from './tags.js';
*/
function authorized(user, action, object) {
// user may be null
assert(!user || user.authzRoles != null, "user.authzRoles is not null if user is not null");
assert(!user || user.authzRoles !== null, "user.authzRoles is not null if user is not null");
assert(actions.has(action), "action is known");
assert(object != null, "object is not null");
assert(object !== null, "object is not null");

/* As a safe guard, anonymous users can only ever read, regardless of
* policies right now. We don't have any use cases right now where letting
Expand Down
2 changes: 1 addition & 1 deletion src/sources/groups.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ class GroupSource extends Source {
let {title, byline, website, showDatasets, showNarratives} = frontMatter;

// Must be an allowed protocol
if (website != null) {
if (website !== null) {
const {protocol} = parseUrl(website) ?? {};
const allowedProtocols = new Set(["https:", "http:", "ftp:", "ftps:", "mailto:"]);
if (!allowedProtocols.has(protocol)) {
Expand Down
2 changes: 1 addition & 1 deletion src/upstream.js
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ function copyHeaders(headerSource, headerNames) {
return Object.fromEntries(
headerNames
.map(name => [name, headerSource.get(name)])
.filter(([name, value]) => value != null && value !== "") // eslint-disable-line no-unused-vars
.filter(([name, value]) => value !== null && value !== "") // eslint-disable-line no-unused-vars
);
}

Expand Down
2 changes: 1 addition & 1 deletion src/utils/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ const parseNarrativeLanguage = (narrative) => {
const normalizeHeaders = (headers) => {
const withValues =
Object.entries(headers)
.filter(([, value]) => value != null && value !== "");
.filter(([, value]) => value !== null && value !== "");

/* Use the WHATWG Headers object to do most of the normalization, including
* lowercasing and combining duplicate headers as appropriate.
Expand Down
2 changes: 1 addition & 1 deletion src/utils/scripts.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ async function run(argv) {
proc.stderr.on("data", data => console.error(data.toString().replace(/\n$/, "")));

proc.on("close", (code, signal) => {
const result = code !== 0 || signal != null
const result = code !== 0 || signal !== null
? reject
: resolve;
return result({code, signal, argv});
Expand Down
18 changes: 15 additions & 3 deletions static-site/.eslintrc.yaml
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
# Prevent parent configuration from being applied.
root: true

plugins:
- "@typescript-eslint"

Expand All @@ -27,6 +24,11 @@ ignorePatterns:
- public/

rules:
# The TypeScript compiler is able to handle usage of type definitions before
# they are defined.
no-use-before-define: off
"@typescript-eslint/no-use-before-define": ["error", { "functions": false, "classes": false, "typedefs": false }]

react/prop-types: off # Remove this override once all props have been typed using PropTypes or TypeScript.
'@next/next/no-img-element': off # Remove this if we use next.js optimisations for <img>
'@next/next/no-html-link-for-pages': off
Expand All @@ -38,6 +40,16 @@ overrides:
rules:
'@typescript-eslint/no-var-requires': off

# React components can use variables before they are defined.
# This isn't great, as jsx/tsx doesn't imply React usage, but it avoids
# unnecessary workarounds such as:
# https://github.com/nextstrain/auspice/commit/af4c742f3855fa90ea292b4882293ebc66ace853
- files:
- '**.[jt]sx'
rules:
no-use-before-define: off
"@typescript-eslint/no-use-before-define": ["error", { "functions": false, "classes": false, "variables": false }]

parserOptions:
sourceType: module
ecmaVersion: 2022
Expand Down
2 changes: 1 addition & 1 deletion static-site/src/components/ExpandableTiles/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export const ExpandableTiles = <AnyTile extends Tile>({tiles, tileWidth, tileHei
function tilesContainerRef(tilesContainer: HTMLDivElement) {
if (!tilesContainer) return;

if(tilesContainerHeight != tilesContainer.clientHeight) {
if(tilesContainerHeight !== tilesContainer.clientHeight) {
setTilesContainerHeight(tilesContainer.clientHeight)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@
// don't do anything if the ref is undefined or the parent is not a div (IndividualResourceContainer)
if (!ref.current
|| !ref.current.parentNode
|| ref.current.parentNode.nodeName != 'DIV') return;
|| ref.current.parentNode.nodeName !== 'DIV') return;

/* The column CSS is great but doesn't allow us to know if an element is at
the top of its column, so we resort to JS */
Expand All @@ -153,8 +153,8 @@
Icon={MdHistory}
text={`${resource.updateCadence.summary} (n=${resource.nVersions})`}
handleClick={() => setModalResource(resource)}
/>

Check failure on line 156 in static-site/src/components/ListResources/IndividualResource.tsx

View workflow job for this annotation

GitHub Actions / lint

'+' should be placed at the beginning of the line
</TooltipWrapper>

Check failure on line 157 in static-site/src/components/ListResources/IndividualResource.tsx

View workflow job for this annotation

GitHub Actions / lint

'+' should be placed at the beginning of the line
)
}

Expand Down
2 changes: 1 addition & 1 deletion static-site/src/components/nav-bar/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ const NavBar = ({ minified }: {

return (
<NavContainer>
{Router.pathname != "/" && <>
{Router.pathname !== "/" && <>
<Logo />
<LogoType />
</>}
Expand Down
Loading