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

Enforce ESLint (JavaScript linting) with pre-commit hooks #11699

Closed
wants to merge 6 commits into from

Conversation

ryanahamilton
Copy link
Contributor

The codebase has existing ESLint JavaScript linting rules configured, but they are not enforced (until now).

  • I've added the eslint check to the pre-commit hooks.
  • Refactored all of the existing files to be compliant with the rules so checks are passing.
  • This is only being ran against external files (for now), as some additional refactoring will be required to get the inline scripting in order as well.
  • I manually tested all of the webserver views that utilize these files and was unable to yield any console errors.

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@boring-cyborg boring-cyborg bot added area:dev-tools area:webserver Webserver related Issues labels Oct 20, 2020
@mik-laj
Copy link
Member

mik-laj commented Oct 20, 2020

Does this eslint configuration meet the project style guide?
https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#follow-javascript-style-guide

var oriSeconds = seconds;
var floatingPart = oriSeconds- Math.floor(oriSeconds);
export function convertSecsToHumanReadable(secs) {
const oriSeconds = secs;
Copy link
Member

@mik-laj mik-laj Oct 20, 2020

Choose a reason for hiding this comment

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

Do we have Babel? I know there were some compatibility issues with older browsers.

@ryanahamilton
Copy link
Contributor Author

@mik-laj yes this is running the same configuration as yarn run lint, it just has a narrower scope (doesn't run against HTML files yet).

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks$,^Build docs$,^Spell check docs$,^Backport packages$,^Checks: Helm tests$,^Test OpenAPI*.

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

/home/runner/work/airflow/airflow/airflow/www/static/js/ie.js
  22:8  error  Unable to resolve path to module 'url-search-params-polyfill'  import/no-unresolved

✖ 2 problems (1 error, 1 warning)


/home/runner/work/airflow/airflow/airflow/www/webpack.config.js
  20:25  error  Unable to resolve path to module 'webpack'                             import/no-unresolved
  22:32  error  Unable to resolve path to module 'webpack-manifest-plugin'             import/no-unresolved
  23:23  error  Unable to resolve path to module 'clean-webpack-plugin'                import/no-unresolved
  24:35  error  Unable to resolve path to module 'copy-webpack-plugin'                 import/no-unresolved
  25:38  error  Unable to resolve path to module 'mini-css-extract-plugin'             import/no-unresolved
  26:37  error  Unable to resolve path to module 'moment-locales-webpack-plugin'       import/no-unresolved
  27:41  error  Unable to resolve path to module 'optimize-css-assets-webpack-plugin'  import/no-unresolved

We might need to use something like https://www.npmjs.com/package/eslint-import-resolver-webpack ?

@ryanahamilton
Copy link
Contributor Author

@ashb pre-commit wasn't including files outside of the /js/ directory locally—I think I just need to add an explicit argument to restrict to that directory—though it might be preferred to do as you suggest and check/fix all of the files?

@potiuk
Copy link
Member

potiuk commented Oct 21, 2020

@ashb pre-commit wasn't including files outside of the /js/ directory locally—I think I just need to add an explicit argument to restrict to that directory—though it might be preferred to do as you suggest and check/fix all of the files?

Yeah - we are running pre-commit with --all-files switch in CI.

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks$,^Build docs$,^Spell check docs$,^Backport packages$,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks$,^Build docs$,^Spell check docs$,^Backport packages$,^Checks: Helm tests$,^Test OpenAPI*.

@kaxil
Copy link
Member

kaxil commented Oct 22, 2020

Rebased as Master was failing

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks$,^Build docs$,^Spell check docs$,^Backport packages$,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks$,^Build docs$,^Spell check docs$,^Backport packages$,^Checks: Helm tests$,^Test OpenAPI*.

@kaxil
Copy link
Member

kaxil commented Oct 22, 2020

@ryanahamilton CI failed with the following:

ESLint............................................................................................................................Failed
- hook id: eslint
- exit code: 1

Error resolving webpackConfig Error: Cannot find module 'webpack'
Require stack:
- /home/runner/work/airflow/airflow/ai
.....

https://github.com/apache/airflow/pull/11699/checks?check_run_id=1292691649#step:7:176

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks$,^Build docs$,^Spell check docs$,^Backport packages$,^Checks: Helm tests$,^Test OpenAPI*.

@ryanahamilton ryanahamilton force-pushed the js_linting branch 2 times, most recently from b0453e1 to 953fb25 Compare November 3, 2020 01:36
@github-actions
Copy link

github-actions bot commented Nov 3, 2020

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions
Copy link

github-actions bot commented Nov 3, 2020

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions
Copy link

github-actions bot commented Nov 3, 2020

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

ashb added a commit to astronomer/airflow that referenced this pull request Nov 3, 2020
This lead to bases such as in apache#11699 where despite there being changes,
and an image being build, the pre-commit tests were not being run.
ashb added a commit that referenced this pull request Nov 3, 2020
This lead to bases such as in #11699 where despite there being changes,
and an image being build, the pre-commit tests were not being run.
@github-actions
Copy link

github-actions bot commented Nov 3, 2020

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions
Copy link

github-actions bot commented Nov 3, 2020

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@kaxil
Copy link
Member

kaxil commented Nov 4, 2020

Can you please rebase your PR on latest Master since we have applied Black and PyUpgrade on Master.

It will help if your squash your commits into single commit first so that there are less conflicts.

potiuk pushed a commit that referenced this pull request Nov 14, 2020
This lead to bases such as in #11699 where despite there being changes,
and an image being build, the pre-commit tests were not being run.

(cherry picked from commit 8000ab7)
potiuk pushed a commit that referenced this pull request Nov 16, 2020
This lead to bases such as in #11699 where despite there being changes,
and an image being build, the pre-commit tests were not being run.

(cherry picked from commit 8000ab7)
potiuk pushed a commit that referenced this pull request Nov 16, 2020
This lead to bases such as in #11699 where despite there being changes,
and an image being build, the pre-commit tests were not being run.

(cherry picked from commit 8000ab7)
kaxil pushed a commit that referenced this pull request Nov 18, 2020
This lead to bases such as in #11699 where despite there being changes,
and an image being build, the pre-commit tests were not being run.

(cherry picked from commit 8000ab7)
@ryanahamilton ryanahamilton added the area:UI Related to UI/UX. For Frontend Developers. label Nov 18, 2020
@github-actions
Copy link

github-actions bot commented Mar 1, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Mar 1, 2021
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
…#12070)

This lead to bases such as in apache#11699 where despite there being changes,
and an image being build, the pre-commit tests were not being run.

(cherry picked from commit 8000ab7)
@github-actions github-actions bot closed this Mar 6, 2021
@ryanahamilton ryanahamilton deleted the js_linting branch June 1, 2021 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues stale Stale PRs per the .github/workflows/stale.yml policy file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants