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 JS parsing logic into JS language #18448

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented May 13, 2024

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:

  • Creation of the VFile class (implements the File interface from the RFC) and tests.
  • Creation of the JS language object (implements the 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.
  • Added language: "@/js" to the default config.
  • Moved languageOptions validation out of the default schema and into the JS language object.
  • Updated flat config array tests to verify all the relocated behavior works.
  • Created a generic 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.

@nzakas nzakas requested a review from a team as a code owner May 13, 2024 21:45
@eslint-github-bot eslint-github-bot bot added the chore This change is not user-facing label May 13, 2024
@github-actions github-actions bot added the core Relates to ESLint's core APIs and features label May 13, 2024
Copy link

netlify bot commented May 13, 2024

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit ff2233c
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/6656125e0f64380008253c46

@aladdin-add aladdin-add added the accepted There is consensus among the team that this change meets the criteria for inclusion label May 16, 2024
@mdjermanovic
Copy link
Member

Sorry for the delay, I plan to review this this week.

//-----------------------------------------------------------------------------

const debug = createDebug("eslint:languages:js");
const DEFAULT_ECMA_VERSION = 5;
Copy link
Member

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?

Copy link
Member Author

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.

lib/languages/js/index.js Outdated Show resolved Hide resolved
lib/languages/js/validate-language-options.js Show resolved Hide resolved
lib/languages/js/source-code/source-code.js Outdated Show resolved Hide resolved
lib/languages/js/source-code/source-code.js Outdated Show resolved Hide resolved
Comment on lines 1189 to 1192
// We only need to do this for ESTree-compatible ASTs
if (!this.isESTree) {
return;
}
Copy link
Member

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.

Copy link
Member Author

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.

lib/linter/linter.js Outdated Show resolved Hide resolved
lib/linter/linter.js Outdated Show resolved Hide resolved
lib/languages/js/index.js Outdated Show resolved Hide resolved
lib/linter/linter.js Outdated Show resolved Hide resolved
@mdjermanovic
Copy link
Member

@christian-bromann looks like there's a problem in WebdriverIO:

[0-0] RUNNING in chrome - file:///tests/lib/linter/linter.js
[0-0]  Error:  Test failed due to following error(s):
  - downloadFile.js?v=74a71965: Uncaught SyntaxError: The requested module '/node_modules/jszip/dist/jszip.min.js?v=74a71965' does not provide an export named 'default'

@christian-bromann
Copy link
Contributor

@mdjermanovic I will take a look tomorrow.

@christian-bromann
Copy link
Contributor

@mdjermanovic please re-run the tests, it should pass now.

@mdjermanovic
Copy link
Member

@christian-bromann now the test fails with:

Execution of 1 workers started at 2024-05-28T06:17:14.648Z

[0-0] RUNNING in chrome - file:///tests/lib/linter/linter.js
[0-0]  Error:  Test failed due to following error(s):
  - linter.js: Timed out after 300s waiting for test results

[0-0] FAILED in chrome - file:///tests/lib/linter/linter.js

 "concise" Reporter:
========= Your concise report ==========
chrome-headless-shell (v125.0.6422.60) on linux
❌ Failed to setup tests, no tests found


Spec Files:	 0 passed, 1 failed, 1 total (100% completed) in 00:05:03 

@christian-bromann
Copy link
Contributor

@mdjermanovic all test started to run extremely slow, I will investigate as to why.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion chore This change is not user-facing core Relates to ESLint's core APIs and features
Projects
Status: Implementing
Development

Successfully merging this pull request may close these issues.

None yet

4 participants