mit-jp / mit-climate-data-viz

Plotting climate data for the MIT Joint Program on the Science and Policy of Global Change
https://cypressf.shinyapps.io/eppa-dashboard/
0 stars 0 forks source link

add eslint, react lint, vscode integration, run on build #236

Closed cypressf closed 2 years ago

cypressf commented 2 years ago

vite doesn't eslint by default, I think—we need to enable it

I'd like to enforce a specific automatic code style, maybe airbnb?

cypressf commented 2 years ago

I added eslint, and I'm trying to configure it to warn me for common react mistakes, and use the airbnb styleguide.

cypressf commented 2 years ago

I'm trying out this guide for prettier + eslint + airbnb styleguide https://vicvijayakumar.com/blog/eslint-airbnb-style-guide-prettier

cypressf commented 2 years ago
cypressf commented 2 years ago

I'm getting two errors with .tsx imports

Unable to resolve path to module './Footer'

https://stackoverflow.com/questions/55198502/using-eslint-with-typescript-unable-to-resolve-path-to-module/56696478#56696478

Missing file extension "tsx" for "./Footer"

https://stackoverflow.com/questions/59265981/typescript-eslint-missing-file-extension-ts-import-extensions

cypressf commented 2 years ago

There's an issue with 'geojson' and 'topojson-specification' imports

code

import { GeoJsonProperties, Feature, Geometry } from 'geojson'
import { GeometryCollection } from 'topojson-specification'

error

Unable to resolve path to module 'geojson'
Unable to resolve path to module 'topojson-specification'

rule

https://github.com/import-js/eslint-plugin-import/blob/v2.25.4/docs/rules/no-unresolved.md

These imports also weren't auto-added by vscode when I typed the classes I was using from them, so I'm guessing there's something fundamentally different about their dependency paths.

solution

I needed to use import type instead of import

cypressf commented 2 years ago

Error with typescript function types (unused var)

I'm seeing this issue

Solution

resolved via https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-unused-vars.md

cypressf commented 2 years ago

jsx types not found

'SVGGElement' is not defined. eslint(no-undef) https://eslint.org/docs/rules/no-undef

I'm not sure why it's failing to find this type. I tried importing it from react as well as React as well as using import type

Solution

This is fixed. I forget which change did it, but it was probably adding the "@typescript-eslint" plugin.

cypressf commented 2 years ago

Lint fails on test files

test-utils.tsx, and .test.tsx files are failing. Part of it is the linter complaining that the test libraries should be included in the project dependencies, not the dev dependencies.

cypressf commented 2 years ago

I added eslint-plugin-jest and tried extends ["plugin:jest/recommended"] and plugins ["jest"] in my .eslintrc.json but that didn't resolve the lint failures in my test files. I'm thinking it's because my test files end with .test.tsx, and are in the same directory as my production code, which isn't what the plugin is looking for.

cypressf commented 2 years ago

I tried adding an .eslintrc overrides for the test file extension:

    "overrides": [
        {
            "files": ["**/*.test.tsx"],
            "env": {
                "jest/globals": true
            },
            "plugins": ["jest"],
            "extends": ["plugin:jest/recommended"]
        }
    ]

but it's still erroring on test methods like test, expect, beforeAll

cypressf commented 2 years ago

I added the following rule for test files based on this stack overflow question and it made eslint stop complaining that I was using dev dependencies in them.

"rules": {
    "import/no-extraneous-dependencies": ["error", { "devDependencies": true }]
}
cypressf commented 2 years ago

Finally, adding this env to my test file overrides made it recognize the built-in jest functions. Source

"env": {
    "jest/globals": true
}
cypressf commented 2 years ago

I also added some testing-library-specific eslint rules via this plugin to enforce testing best-practices.

cypressf commented 2 years ago

I noticed an issue

"env": {
    "jest/globals": true
}

was causing no eslint rules to be highlighted in vscode. I think it's valid only if you install the eslint jest plugin, which I did not. instead, I'm using

"env": {
    "jest": true
}

which seems to work just fine.

cypressf commented 2 years ago

I set up User/settings.json in vscode to automatically apply prettier format and eslint auto-fixes on save.

  "[typescriptreact]": {
    "editor.defaultFormatter": "esbenp.prettier-vscode"
  },
  "[javascript]": {
    "editor.defaultFormatter": "esbenp.prettier-vscode"
  },
  "[typescript]": {
    "editor.defaultFormatter": "esbenp.prettier-vscode"
  },
  "[jsonc]": {
    "editor.defaultFormatter": "esbenp.prettier-vscode"
  },
  "editor.formatOnSave": true,
  "editor.codeActionsOnSave": {
    "source.fixAll.eslint": true
  },
cypressf commented 2 years ago

Now eslint is happy with the entire codebase, so we're down to the

homestretch: configure eslint to run when building with vite

I can use the vite-plugin-eslint to run on build. I'm unsure if this is what I want, or if I should just add an extra step to the production build that lints. For now, let's try the plugin.

cypressf commented 2 years ago

I added the vite plugin, and it's giving me errors that some variables are not in camel_case that the built-in eslint in vscode is not giving me. I'm investigating why there's a discrepancy. I would expect vscode's eslint to also show errors for that.

cypressf commented 2 years ago

I get camel case errors in vscode if I modify variables defined in Home.tsx. Why isn't it giving me errors about the MapVisualization.tsx variables?

cypressf commented 2 years ago

I'm using https://www.npmjs.com/package/@nabla/vite-plugin-eslint instead and it seems to correctly identify only the errors identified by vscode. The other plugin must not have been loading my eslintrc correctly.