-
Notifications
You must be signed in to change notification settings - Fork 178
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
chore(share-data): add format and lint targets for js #16529
Conversation
I don't think our JS projects are usually set up to have project-specific lint and format targets, are they? It's just the targets in the top-level makefile. Like, |
Hmm, that is true, but do you know why it is so? Maybe it isn't much of difference if you want to use the lint and format targets from root level when linting/formatting bigger projects like app or app-shell, but to run linter/formatter on the full monorepo after making a small change in a json file of a small project like shared-data feels excessive to me. I know I can just use the yarn prettier command, but this is just making the same command easier to use (and not have to look it up every time because I use it only once every couple of months and immediately forget about it 😬). |
I don't know—I'd love a JS person to chime in—but I get the sense that they just think of projects differently overall. In Python-land, we have separate environments, separate IDE projects, and separate tool configurations for all of our subprojects. In JS-land, all the subprojects share one environment (I think? There is only one, top-level, yarn.lock file), one set of tool configs (e.g. one top-level .eslintrc.js file), and you can just open the whole monorepo as a single IDE project. |
shared-data/Makefile
Outdated
yarn prettier --ignore-path .eslintignore --write $(FORMAT_FILE_GLOB) | ||
|
||
.PHONY: lint-js | ||
lint-js: |
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.
missing eslint, yarn eslint
@@ -9,20 +9,25 @@ tests ?= | |||
cov_opts ?= --coverage=true | |||
test_opts ?= | |||
|
|||
FORMAT_FILE_GLOB = "**/*.@(ts|tsx|js|json|md|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.
need to add 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.
I guess it doesn't hurt..?
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.
yeah, it won't cause any issue.
shared-data/.eslintignore
Outdated
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.
You may not need to add this, I think it will use the top level one directory up but try it out!
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.
thanks!
shared-data/Makefile
Outdated
|
||
.PHONY: lint-js-eslint | ||
lint-js-eslint: | ||
yarn eslint --ignore-path ../.eslintignore --quiet=$(quiet) --ignore-pattern "node_modules/" "**/*.@(js|ts|tsx)" |
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.
--ignore-pattern "node_modules/" "**/*.@(js|ts|tsx)"
should already be taken care by whats in the . eslintignore
right?
Overview
Adds
format-js
andlint-js
targets to shared-data's makefile. Also adds the ignore rules file for it.Review requests
setup-js
necessary for this? The phony target was listed in the makefile but but without any recipe.Risk assessment
None