chore: improve/decouple eslint and tsc 'npm run' commands #26704

mistercrunch · 2024-01-20T22:19:06Z

SUMMARY

I was having issues running just tsc or eslint using our npm run scripts in package.json, because of the way they were set up. Comment line-per-line in the PR here.

main goal of the PR: improve npm run commands
secondary mission: break down some linting CI steps into sub-step (one for eslint, and one for tsc), allowing better time accounting and error/warning attribution

mistercrunch · 2024-01-20T22:20:28Z

superset-frontend/package.json

@@ -37,7 +37,7 @@
    "src/setup/*"
  ],
  "scripts": {
-    "_lint": "eslint --ignore-path=.eslintignore --ext .js,.jsx,.ts,tsx .",
+    "eslint": "eslint --ignore-path=.eslintignore --ext .js,.jsx,.ts,tsx",


killing _lint with a simple eslint command with the parameters that should always be used, but importantly removing that . at the end that prevents using this construct to run a specific folder of file. Since _lint is implicitely "private" given the underscore, I thought is was ok to just remove it.

mistercrunch · 2024-01-20T22:21:57Z

superset-frontend/package.json

@@ -51,8 +51,8 @@
    "dev": "webpack --mode=development --color --watch",
    "dev-server": "cross-env NODE_ENV=development BABEL_ENV=development node --max_old_space_size=4096 ./node_modules/webpack-dev-server/bin/webpack-dev-server.js --mode=development",
    "format": "npm run _prettier -- --write",
-    "lint": "npm run _lint && npm run type",
-    "lint-fix": "npm run _lint -- --fix && npm run type",
+    "lint": "npm run eslint -- . && npm run type",


maintaining backward compatibility, though I think we should deprecate eventually as it's unclear that you're running two commands here, and arguably tsc isn't really linting.

mistercrunch · 2024-01-20T22:22:18Z

superset-frontend/package.json

-    "lint": "npm run _lint && npm run type",
-    "lint-fix": "npm run _lint -- --fix && npm run type",
+    "lint": "npm run eslint -- . && npm run type",
+    "lint-fix": "npm run eslint -- . --fix",


same here, maintaining backward compat

mistercrunch · 2024-01-20T22:24:50Z

.github/workflows/superset-frontend.yml

@@ -36,11 +36,20 @@ jobs:
        uses: ./.github/actions/cached-dependencies
        with:
          run: npm-install
-      - name: lint
+      - name: eslint


breaking down the steps in CI makes it more clear where the output is coming from, and gets timing info on the different steps. Helping understand the time it takes for each step

codecov · 2024-01-21T07:17:58Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e86d4d3) 69.48% compared to head (b2215e4) 69.48%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #26704   +/-   ##
=======================================
  Coverage   69.48%   69.48%           
=======================================
  Files        1894     1894           
  Lines       74138    74138           
  Branches     8243     8243           
=======================================
  Hits        51515    51515           
  Misses      20554    20554           
  Partials     2069     2069

Flag	Coverage Δ
javascript	`56.86% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

john-bodley · 2024-01-23T17:50:35Z

superset-frontend/js_build.sh

@@ -20,6 +20,7 @@ cd "$(dirname "$0")"
 npm --version
 node --version
 time npm ci
-time npm run lint
+time npm run eslint -- .


Why is -- . needed here and not for TypeScript?

-- from my understanding is a npm run feature where it allows you to pass arguments to the underlying command. npm will basically concatenate what's on the right of -- with the script / string you define. The way it was before where npm run lint would wrap two commands, and with npm run _lint it would specify the ., so there was no way to use these pre-made scripts and "overload" your own arguments on top of the predefined ones.

In this case, I designed the npm run eslint to not include the . like it did before, so that a user that knows these conventions could run npm run eslint -- myfile.js or npm run eslint -- --fix ..

About the question about the . not being needed for tsc, it's just that somehow npm run type script is tsc --noEmit, it doesn't require a file argument like eslint does.

geido

Thanks for this!

mistercrunch added 3 commits January 20, 2024 13:40

chore: fix GitHub 'Unchanged files with check annotations' reports in PR

0e6e702

backward compatible npm run lint-fix

e105447

chore: improve/decouple eslint and tsc 'npm run' commands

a254231

mistercrunch requested review from villebro, geido, eschutho, rusackas, betodealmeida, nytai, craig-rueda, john-bodley, kgabryje and jinghua-qa as code owners January 20, 2024 22:19

pull-request-size bot added the size/S label Jan 20, 2024

mistercrunch commented Jan 20, 2024

View reviewed changes

breaking down the steps in CI

bba1863

pull-request-size bot added size/M and removed size/S labels Jan 20, 2024

mistercrunch commented Jan 20, 2024

View reviewed changes

mistercrunch added 3 commits January 20, 2024 14:27

fix

7babe8c

touchups

d2bdf18

wot

b2215e4

john-bodley reviewed Jan 23, 2024

View reviewed changes

geido approved these changes Jan 24, 2024

View reviewed changes

mistercrunch merged commit 0f59079 into apache:master Jan 24, 2024
35 checks passed

mistercrunch deleted the improve_npm_commands_lint branch January 24, 2024 16:51

mistercrunch mentioned this pull request Jan 24, 2024

chore: fix GitHub 'Unchanged files with check annotations' reports in PR #26702

Merged

rusackas pushed a commit that referenced this pull request Jan 24, 2024

chore: improve/decouple eslint and tsc 'npm run' commands (#26704)

6b21b73

eschutho pushed a commit to preset-io/superset that referenced this pull request Jan 31, 2024

chore: improve/decouple eslint and tsc 'npm run' commands (apache#26704)

b3ec864

sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024

chore: improve/decouple eslint and tsc 'npm run' commands (apache#26704)

03dbd7d

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 4.0.0 labels Apr 17, 2024

vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024

chore: improve/decouple eslint and tsc 'npm run' commands (apache#26704)

b2b29ab

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: improve/decouple eslint and tsc 'npm run' commands #26704

chore: improve/decouple eslint and tsc 'npm run' commands #26704

mistercrunch commented Jan 20, 2024 •

edited by geido

Loading

mistercrunch Jan 20, 2024 •

edited

Loading

mistercrunch Jan 20, 2024

mistercrunch Jan 20, 2024

mistercrunch Jan 20, 2024

codecov bot commented Jan 21, 2024

john-bodley Jan 23, 2024

mistercrunch Jan 23, 2024 •

edited

Loading

geido left a comment

chore: improve/decouple eslint and tsc 'npm run' commands #26704

chore: improve/decouple eslint and tsc 'npm run' commands #26704

Conversation

mistercrunch commented Jan 20, 2024 • edited by geido Loading

SUMMARY

mistercrunch Jan 20, 2024 • edited Loading

Choose a reason for hiding this comment

mistercrunch Jan 20, 2024

Choose a reason for hiding this comment

mistercrunch Jan 20, 2024

Choose a reason for hiding this comment

mistercrunch Jan 20, 2024

Choose a reason for hiding this comment

codecov bot commented Jan 21, 2024

Codecov Report

john-bodley Jan 23, 2024

Choose a reason for hiding this comment

mistercrunch Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

geido left a comment

Choose a reason for hiding this comment

mistercrunch commented Jan 20, 2024 •

edited by geido

Loading

mistercrunch Jan 20, 2024 •

edited

Loading

mistercrunch Jan 23, 2024 •

edited

Loading