-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add eslint and JavaScript linting in GitHub Actions #36
Conversation
push: | ||
branches: | ||
- trunk | ||
paths: | ||
- "**.js" | ||
- "**.mjs" | ||
- .github/workflows/js-linting.yml | ||
pull_request: | ||
paths: | ||
- "**.js" | ||
- "**.mjs" | ||
- .github/workflows/js-linting.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💅 Maybe running this WorkFlow on PR against trunk would be enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the linting check only runs on PR against trunk, PR to PR, like this current one (#34), wouldn't be run. I think finding out linting errors earlier should be ideal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant is to remove the push
directive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The temporarily merged code of a PR to be run checks is not always the same as the merged one after a PR actually gets merged.
For example, if there are other commits are pushed to trunk after this PR was opened, this PR won't be triggered and run the check again if there are no conflicts. When two PR change the same file like this:
const config = {
+ hello: '123', // Added by PR A
howdy: '000',
+ hello: '456', // Added by PR B
};
};
Both PR could pass the check, but it would be a problem after merging. When this happens, the check run on push events of trunk might find out this problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right! Thanks for the explanation 👌
@@ -0,0 +1,43 @@ | |||
name: JavaScript Linting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💅 If we are going to set up a global package.json I'd also add a .nvmrc
file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in fa4b346.
@puntope, thanks for the review.
Thanks for catching this. Seems it results in a different node_modules structure in the initial install, and somehow the flatted structure of package-lock after running Could you help with another round of code reviews? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ LGTM
- Tested
lint:js
locally. I checked forcing some errors. - Checked the action check
- Checked the code quality
Address #36 (comment)
99ac8ce
to
f8eff65
Compare
Changes proposed in this Pull Request:
Implements parts of #4
eslint
, related deps and their configurations.Before merging this PR:
eslint
andstylelint
for annotating the linting results via their formatter #35 to be released.actions-v1-pre
toactions-v1
.Detailed test instructions:
npm install
npm run lint:js
locally to test the eslintimport/no-unresolved
warnings because the node_modules directories in sub-packages don't exist. Those warnings will disappear after runningnpm install
in those sub-packages.