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.

Sign up for GitHub

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

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

Merged
merged 7 commits into from
Jan 24, 2024
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0e6e702
chore: fix GitHub 'Unchanged files with check annotations' reports in PR
mistercrunch Jan 20, 2024
e105447
backward compatible npm run lint-fix
mistercrunch Jan 20, 2024
a254231
chore: improve/decouple eslint and tsc 'npm run' commands
mistercrunch Jan 20, 2024
bba1863
breaking down the steps in CI
mistercrunch Jan 20, 2024
7babe8c
fix
mistercrunch Jan 20, 2024
d2bdf18
touchups
mistercrunch Jan 20, 2024
b2215e4
wot
mistercrunch Jan 21, 2024
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
13 changes: 11 additions & 2 deletions .github/workflows/superset-frontend.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,20 @@ jobs:
uses: ./.github/actions/cached-dependencies
with:
run: npm-install
- name: lint
- name: eslint
Copy link
Member Author

Choose a reason for hiding this comment

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

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

if: steps.check.outcome == 'failure'
working-directory: ./superset-frontend
run: |
npm run eslint -- . --quiet
- name: tsc
if: steps.check.outcome == 'failure'
working-directory: ./superset-frontend
run: |
npm run type
- name: prettier
if: steps.check.outcome == 'failure'
working-directory: ./superset-frontend
run: |
npm run lint -- --quiet
npm run prettier-check
- name: Build plugins packages
if: steps.check.outcome == 'failure'
Expand Down
7 changes: 5 additions & 2 deletions .github/workflows/superset-websocket.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,12 @@ jobs:
- name: Install dependencies
working-directory: ./superset-websocket
run: npm ci
- name: lint
- name: eslint
working-directory: ./superset-websocket
run: npm run lint
run: npm run eslint -- .
- name: typescript checks
working-directory: ./superset-websocket
run: npm run type
- name: prettier
working-directory: ./superset-websocket
run: npm run prettier-check
Expand Down
5 changes: 4 additions & 1 deletion CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -788,7 +788,10 @@ is configured as a pre-commit hook. There are also numerous [editor integrations
```bash
cd superset-frontend
npm ci
npm run lint
# run eslint checks
npm run eslint -- .
# run tsc (typescript) checks
npm run type
```

If using the eslint extension with vscode, put the following in your workspace `settings.json` file:
Expand Down
3 changes: 3 additions & 0 deletions docs/docs/contributing/hooks-and-linting.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,10 @@ is configured as a pre-commit hook. There are also numerous [editor integrations
```bash
cd superset-frontend
npm ci
# run eslint checks
npm run lint
# run tsc (typescript) checks
npm run type
```

If using the eslint extension with vscode, put the following in your workspace `settings.json` file:
Expand Down
2 changes: 2 additions & 0 deletions superset-frontend/cypress-base/cypress/support/e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import '@cypress/code-coverage/support';
import '@applitools/eyes-cypress/commands';
import failOnConsoleError from 'cypress-fail-on-console-error';

/* eslint-disable @typescript-eslint/no-explicit-any */

require('cy-verify-downloads').addCustomCommand();

// fail on console error, allow config to override individual tests
Expand Down
1 change: 1 addition & 0 deletions superset-frontend/js_build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,6 @@ npm --version
node --version
time npm ci
time npm run lint
time npm run check
time npm run cover # this also runs the tests, so no need to 'npm run test'
time npm run build
6 changes: 3 additions & 3 deletions superset-frontend/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Member Author

@mistercrunch mistercrunch Jan 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

"_prettier": "prettier './({src,spec,cypress-base,plugins,packages,.storybook}/**/*{.js,.jsx,.ts,.tsx,.css,.less,.scss,.sass}|package.json)'",
"build": "cross-env NODE_OPTIONS=--max_old_space_size=8192 NODE_ENV=production BABEL_ENV=\"${BABEL_ENV:=production}\" webpack --mode=production --color",
"build-dev": "cross-env NODE_OPTIONS=--max_old_space_size=8192 NODE_ENV=development webpack --mode=development --color",
Expand All @@ -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",
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

"lint-fix": "npm run eslint -- . --fix",
Copy link
Member Author

Choose a reason for hiding this comment

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

same here, maintaining backward compat

"plugins:build": "node ./scripts/build.js",
"plugins:build-assets": "node ./scripts/copyAssets.js",
"plugins:build-storybook": "cd packages/superset-ui-demo && npm run build-storybook",
Expand Down
Loading