-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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 JS parsing logic into JS language #18448
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for docs-eslint canceled.
|
Sorry for the delay, I plan to review this this week. |
//----------------------------------------------------------------------------- | ||
|
||
const debug = createDebug("eslint:languages:js"); | ||
const DEFAULT_ECMA_VERSION = 5; |
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.
to confirm: DEFAULT_ECMA_VERSION
set to 5, for eslintrc-compat?
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.
That's correct. The eslintrc branch of logic will still use this file, so want to cover our bases.
// We only need to do this for ESTree-compatible ASTs | ||
if (!this.isESTree) { | ||
return; | ||
} |
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 might be a breaking change because this code was executed for non-estree ASTs.
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.
Interesting. Without this, several tests failed, so I thought we were skipping logic. I'll take another look.
Co-authored-by: Milos Djermanovic <[email protected]>
Co-authored-by: Milos Djermanovic <[email protected]>
Co-authored-by: Milos Djermanovic <[email protected]>
Co-authored-by: Milos Djermanovic <[email protected]>
@christian-bromann looks like there's a problem in WebdriverIO:
|
@mdjermanovic I will take a look tomorrow. |
@mdjermanovic please re-run the tests, it should pass now. |
@christian-bromann now the test fails with:
|
@mdjermanovic all test started to run extremely slow, I will investigate as to why. |
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[x] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
This PR moves JS parsing and config handling logic into a new JS language object (
/lib/languages/js/index.js
), and includes:VFile
class (implements theFile
interface from the RFC) and tests.Language
interface). This deviates slightly from the RFC in that I've renamed "options" in all places to "languageOptions" to avoid confusion. Otherwise, I was constantly assigning back and forth between these two names.language: "@/js"
to the default config.languageOptions
validation out of the default schema and into the JS language object.languageOptionsSchema
that does not validate the keys (that is now done in the JS language object) but preserves the merge behavior.Refs #16999
Is there anything you'd like reviewers to focus on?
I think these changes are backwards-compatible, but would like another set of eyes.
languageOptionsSchema
is the part I'm most concerned about. The previous merging behavior was very JS-specific, so I tried to come up with a generic version that would make sense for other languages. Please double-check that.