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

Merged
merged 31 commits into from
Jun 14, 2024
Merged

refactor: Move JS parsing logic into JS language #18448

merged 31 commits into from
Jun 14, 2024

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 94b2d28
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/666b0929935e880008d26881

@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
lib/languages/js/source-code/source-code.js Outdated Show resolved Hide resolved
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.

lib/linter/linter.js Outdated Show resolved Hide resolved
lib/languages/js/source-code/source-code.js Outdated Show resolved Hide resolved
tests/lib/linter/linter.js Outdated Show resolved Hide resolved
tests/lib/linter/linter.js Outdated Show resolved Hide resolved
tests/lib/linter/linter.js Outdated Show resolved Hide resolved
tests/lib/eslint/eslint.js Outdated Show resolved Hide resolved
@nzakas
Copy link
Member Author

nzakas commented May 31, 2024

@christian-bromann any update on these wdio failures? We've not had a test pass in several days and it's becoming an issue when reviewing PRs.

@aladdin-add
Copy link
Member

ci failing has been fixed in the main branch, can you please rebase it, thanks!

@christian-bromann
Copy link
Contributor

@nzakas sorry, I was out for some days, I will attempt to find a resolution today.

@christian-bromann
Copy link
Contributor

christian-bromann commented Jun 4, 2024

CI / Browser Test (pull_request) Successful in 2m

🙌

fyi: we are adding Eslint browser test to our WebdriverIO pipeline as recommend by @nzakas as we have done some improvements on the pipeline which now allows us to make this simpler.

@mdjermanovic
Copy link
Member

@christian-bromann is the problem fixed in @wdio/browser-runner v8.38.2?

(we temporarily pinned @wdio/browser-runner to v8.36.0, and now the browser test sometimes works, sometimes doesn't)

@christian-bromann
Copy link
Contributor

christian-bromann commented Jun 5, 2024

is the problem fixed in @wdio/browser-runner v8.38.2?

I just published v8.38.3 with a definitive fix, raised a PR to Eslint with an update to all WDIO deps: #18558

@nzakas
Copy link
Member Author

nzakas commented Jun 7, 2024

This is ready for re-review.

@mdjermanovic
Copy link
Member

I left #18448 (comment)

@nzakas
Copy link
Member Author

nzakas commented Jun 10, 2024

@mdjermanovic Sorry, missed that. Should be set now.

lib/linter/linter.js Outdated Show resolved Hide resolved
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! I'm only not sure if disable directives would work well with languages that have lineStart: 0, but will doublecheck that and submit a follow-up PR if needed.

@mdjermanovic mdjermanovic merged commit 4b23ffd into main Jun 14, 2024
18 checks passed
@mdjermanovic mdjermanovic deleted the jslanguage branch June 14, 2024 09:26
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: Complete
Development

Successfully merging this pull request may close these issues.

None yet

4 participants