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

refactor: move linting from root to each lib #24424

Merged
merged 1 commit into from Nov 17, 2022

Conversation

jordanpowell88
Copy link
Contributor

@jordanpowell88 jordanpowell88 commented Oct 27, 2022

User facing changelog

n/a

Additional details

  • Currently we lint the entire repo at the root level with one giant .eslintignore file. Ideally these are in individual libs to narrow the scope of executing the lint script.
  • This also prefaces to be able to leverage caching these lint scripts in the future when we update to using Lerna 6. - 23688

Steps to test

yarn lint

How has the user experience changed?

n/a

PR Tasks

  • Have tests been added/updated?
  • Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • Has a PR for user-facing changes been opened in cypress-documentation?
  • Have API changes been updated in the type definitions?

@cypress-bot
Copy link

cypress-bot bot commented Oct 27, 2022

Thanks for taking the time to open a PR!

@jordanpowell88 jordanpowell88 requested a review from flotwig Oct 27, 2022
@cypress
Copy link

cypress bot commented Oct 27, 2022

@jordanpowell88 jordanpowell88 changed the title ref: move linting from root to each lib refactor: move linting from root to each lib Oct 27, 2022
@jordanpowell88 jordanpowell88 force-pushed the jordanpowell88/ref-linting branch 2 times, most recently from 75b410b to 9c7dc54 Compare Nov 1, 2022
@@ -12,7 +12,8 @@
"cypress:open": "yarn cypress:run-cypress-in-cypress gulp open --project .",
"watch": "tsc -w",
"test": "yarn test-unit",
"test-unit": "mocha -r ts-node/register/transpile-only --config ./test/.mocharc.js"
"test-unit": "mocha -r ts-node/register/transpile-only --config ./test/.mocharc.js",
"lint": "eslint --ext .js,.ts,.json, ."
Copy link
Member

Choose a reason for hiding this comment

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

Most of these .eslintignore files are 1:1. Can you do eslint --ignore-path -- ../../.eslintignore when these are shared and using custom .eslintignore files when needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certainly could be ignored from a shared .eslintignore file at the root. My first goal was two-fold:

  • move to linting to individual the lib level
  • avoid having a massive .eslintignore file at the root of the workspace

Copy link
Member

Choose a reason for hiding this comment

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

What is the benefit of having lots of tiny files?

Ideally these are in individual libs to narrow the scope of executing the lint script

Does this actually mean eslint will have to execute on less files? Is there some perf improvement here?

I don't really see the benefit of having more files - I actually prefer one file, so I can just add all my linting rules there. Right now every package has a slightly different set of rules, if anything wouldn't want to move towards 1 eslint file and one eslintignore file for everything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@emilyrohrbough @lmiller1990 The benefit is that later, when we update Lerna (to utilize Nx) we can utilize caching. This way we only have to lint thing that have been affected by changes. Right now we do this once at the root level and therefore we will have to lint essentially every single time on any change

Copy link
Member

Choose a reason for hiding this comment

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

@jordanpowell88 to use nx we need the additional files or do we just need to lint command in each subpackage/workspace?

Copy link
Member

Choose a reason for hiding this comment

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

I am not familiar with the intricacies of Nx caching - if this gets us closer to a better monorepo DX, happy to bikeshed my opinion of "all the linting rules in one file" in favor of moving forward.

I wonder if we can have some way to extend a base set of ignored files? Eg a linting package or something could expose it.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, followed up with @jordanpowell88. We don't actually need separate . eslintignore files for each workspace to have it's own linting script & to use nx caching. Sounds like there are performance implications.

if you change the root estlintignore file it would invalidate the cache and essentially require a re-run of everything to lint (not a huge deal if it doesn't happen often) but if I just change one line for one lib then it still affects performance

@flotwig flotwig requested a review from lmiller1990 Nov 9, 2022
@lmiller1990
Copy link
Member

All for making our linting less complex, but I'm not sure I see the benefit here - if anything, spreading things out seems like more complexity to manage? I could be missing something, though, like some perf or dependency benefits.

Copy link
Member

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Quick comment, not sure I fully grok the benefit/improvement here.

@lmiller1990 lmiller1990 self-requested a review Nov 15, 2022
Copy link
Member

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Going to approve, not the biggest fan of tons of little files but in the spirit of improving the monorepo DX, if this helps with that, I am on board.

@jordanpowell88 jordanpowell88 force-pushed the jordanpowell88/ref-linting branch from a596b6a to 03c71d9 Compare Nov 16, 2022
@lmiller1990 lmiller1990 merged commit ed90b14 into develop Nov 17, 2022
81 of 83 checks passed
@lmiller1990 lmiller1990 deleted the jordanpowell88/ref-linting branch Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants