-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
fix(parser): error when both projectService and project are set
#11333
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
base: main
Are you sure you want to change the base?
Conversation
|
Thanks for the PR, @MariaSolOs! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
View your CI Pipeline Execution ↗ for commit 8363f90
☁️ Nx Cloud last updated this comment at |
d4979a5 to
a8c4dc1
Compare
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.
These are just notes from quick preliminary review. I think we have a good path forward but should definitely let other team folks weigh in in #11319.
For now, I'll switch this PR to draft so it doesn't show up on the queue. But we can definitely un-draft it once the issue's discussion is done.
packages/typescript-estree/src/parseSettings/createParseSettings.ts
Outdated
Show resolved
Hide resolved
|
@JoshuaKGoldberg Thanks for the review! I'll wait for further thoughts from other maintainers before I continue working on this :) |
a8c4dc1 to
4ff2bd3
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11333 +/- ##
=======================================
Coverage 90.95% 90.96%
=======================================
Files 506 506
Lines 51433 51443 +10
Branches 8491 8495 +4
=======================================
+ Hits 46783 46793 +10
Misses 4637 4637
Partials 13 13
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
4ff2bd3 to
63d37d4
Compare
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 source code change looks great. Just requesting adding tests: you should be able to use the existing vi.stubEnv-powered TYPESCRIPT_ESLINT_PROJECT_SERVICE ones around projectService in createParseSettings.test.ts as reference. LMK if that actually doesn't work though!
8f2fe95 to
65acef6
Compare
|
@JoshuaKGoldberg I'm having trouble fixing these tests as I can't reproduce the errors locally. I've been running |
|
Ah, yes, these come from explicitly setting typescript-eslint/.github/workflows/ci.yml Lines 244 to 245 in eb4ecd8
typescript-eslint/.github/workflows/ci.yml Lines 267 to 271 in eb4ecd8
I normally run Vitest directly when only a handful of files are failing: TYPESCRIPT_ESLINT_PROJECT_SERVICE=true yarn vitest packages/eslint-plugin-internal/tests/rules/plugin-test-formatting.test.ts |
65acef6 to
9c9c9b8
Compare
|
@JoshuaKGoldberg got it, thank you for the hint! |
f3f5baa to
cf71e1a
Compare
| languageOptions: { | ||
| parserOptions: { | ||
| project: './tsconfig.json', | ||
| projectService: false, |
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.
Not sure if this is the right way to fix the tests in files that just use the RuleTester.
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.
Hmm, good question. Ideally we wouldn't have to do anything in the plugin tests themselves. I'm poking around...
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.
OK, yes, this is the right way.
Recapping for posterity: it's not apparent from the naming (sigh) but the TYPESCRIPT_ESLINT_PROJECT_SERVICE env variable's purpose isn't purely to enable projectService. It's to switch project to projectService if project is enabled. But it doesn't apply when project: false is present.
cf71e1a to
c2c6ca1
Compare
|
Ok, I think I have a handle on things and am most of the way through normalizing test files to handle the change. I have to step away, but in the meantime filed #11676. Expect more soon. Fun stuff! 🫠 |
Thanks a lot for the help here! <3 |
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.
This was a lot more work than I was expecting it to be 🙃. Sorry for "surprise!" gifting you this big of a task @MariaSolOs!
Summarizing what's going on:
- Previously, if
process.env.TYPESCRIPT_ESLINT_PROJECT_SERVICEwas enabled, rule tests withprojectset were being switched toprojectService- This is why some of them set
projectService: false-- to prevent that switching - See logic in
createParseSettings.ts
- This is why some of them set
- With this PR's intended change, that use case errors intentionally
- I switched typed rule tests from manually calling
new RuleTesterto using a helpercreateRuleTesterWithTypesthat:- If it receives an enabled
project, setsprojectService: falselike before - Enables
projectServiceifprocess.env.TYPESCRIPT_ESLINT_PROJECT_SERVICE - Otherwise enables
project
- If it receives an enabled
| languageOptions: { | ||
| parserOptions: { | ||
| project: './tsconfig.json', | ||
| projectService: false, |
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.
OK, yes, this is the right way.
Recapping for posterity: it's not apparent from the naming (sigh) but the TYPESCRIPT_ESLINT_PROJECT_SERVICE env variable's purpose isn't purely to enable projectService. It's to switch project to projectService if project is enabled. But it doesn't apply when project: false is present.
| // Finally, default to project: true as the standard (legacy) behavior | ||
| // See: https://github.com/typescript-eslint/typescript-eslint/issues/11676 | ||
| else { | ||
| parserOptions.project = true; | ||
| } |
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.
Linking #11676 here so we don't forget: if & when we remove eslint-plugin from the TYPESCRIPT_ESLINT_PROJECT_SERVICE matrix, we could switch the default to projectService: true...
No worries at all! At least I'm relieved that something non-trivial was going on and it wasn't just me having skill issues 😂 Oh my, that
You're awesome, thank you for all the work here! 🦸 |
I hope not... https://sourcegraph.com/search?q=context:global+TYPESCRIPT_ESLINT_PROJECT_SERVICE+-file:.*node_modules.*+-repo:%5Egithub%5C.com/typescript-eslint/typescript-eslint%24+&patternType=keyword&sm=0 shows none. I suppose we'll never know for sure though. 🤷 |

PR Checklist
projectServiceandprojectare set #11319Overview
As mentioned in the issue, the docs aren't clear of what the parser will use when both
projectServiceandprojectare set. When such case is detected, I think it's better to be explicit about the misconfiguration and error as early as possible.