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

Flat ESLint support #12679

Merged
merged 2 commits into from Apr 22, 2024
Merged

Conversation

MadSandwich
Copy link
Contributor

@MadSandwich MadSandwich commented Apr 14, 2024

Proposed changes

ESLint version: 8.57.0

  • cjs support
  • esm support
  • TS (with restrictions)

ESLint version: 9.0.0

  • cjs
  • esm
  • TS (with restrictions)

What have been done: Using migration guide and some repositories where such changes was already introduced, made some changes to index.ts & package.json files. Result was tested on several repositories using different types of configuration, no issues where detected, except while tested with TS (typescript-eslint/typescript-eslint#8211), it working well except it fully ignoring globals.

To make rule work against globals, 'no-undef' rule should be added (TS only).

image
image

p.s. I can attach screenshots with my tests, if it would help anyhow. But it would be great if somebody can take a look on it, before merge.

Types of changes

  • Polish (an improvement for an existing feature)
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update (improvements to the project's docs)
  • Specification changes (updates to WebDriver command specifications)
  • Internal updates (everything related to internal scripts, governance documentation and CI files)

Checklist

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary documentation (if appropriate)
  • I have added proper type definitions for new commands (if appropriate)

Backport Request

//: # (The current main branch is the development branch for WebdriverIO v9. If your change should be released to the current major version of WebdriverIO (v8), please raise another PR with the same changes against the v8 branch.)

Further comments

Reviewers: @webdriverio/project-committers

@MadSandwich MadSandwich changed the base branch from v8 to main April 14, 2024 21:40
@MadSandwich
Copy link
Contributor Author

This PR is continue of the feature described in #12547
And first PR created #12580 (As work stop in it, I've created a separate)

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

I think this would be a good time to reference Eslint as a peer dependency to ensure that someone using this package also has Eslint v9 installed.

Wdyt?

@MadSandwich
Copy link
Contributor Author

I think this would be a good time to reference Eslint as a peer dependency to ensure that someone using this package also has Eslint v9 installed.

Wdyt?

This would make people that have a desire to stay on the previous version of eslint 8.57.0 got an error (after package update). As per eslint 9.0.0 they support only flat config, and other config are deprecated. We can reference it as a peer dep, but in that case most of work to make it compatible with 8.57.0 version would be useless.

@gavvvr
Copy link
Contributor

gavvvr commented Apr 15, 2024

to reference Eslint as a peer dependency to ensure that someone using this package also has Eslint v9 installed.

@christian-bromann I would not enforce consumers to use eslint 9+, if legacy config is still supported (which should be left supported for gradual migration)
I do not even think that eslint dependency should be upgraded within this PR. Both configs are exported now, which should be compatible with both worlds (legacy and flat config system).

@MadSandwich could you please add more details on changes in package.json. Why modifying only a single index.ts is not enough?

@MadSandwich
Copy link
Contributor Author

MadSandwich commented Apr 15, 2024

to reference Eslint as a peer dependency to ensure that someone using this package also has Eslint v9 installed.

@christian-bromann I would not enforce consumers to use eslint 9+, if legacy config is still supported (which should be left supported for gradual migration) I do not even think that eslint dependency should be upgraded within this PR. Both configs are exported now, which should be compatible with both worlds (legacy and flat config system).

@MadSandwich could you please add more details on changes in package.json. Why modifying only a single index.ts is not enough?

Sure,

"type": "module" - was removed to make it easy for exporting for CJS and ESM, as it allow export = {...}, without this it would require 2 exports and issues then in configuration parsing.

This part was changed, to implicit specify where CJS and ESM should pick up their imports.

  "exports": {
    "import": {
      "types": "./build/index.d.ts",
      "default": "./build/index.js"
    },
    "require": {
      "types": "./build/index.d.ts",
      "default": "./cjs/index.js"
    }
  },

And that part was changed, just to be sure, that written tests inside wdio still would work and rules are not affected.
"eslint": "^9.0.0"

@MadSandwich
Copy link
Contributor Author

@christian-bromann what do u think, on my comments above?

@christian-bromann
Copy link
Member

@MadSandwich the change looks good to me. It would be awesome if we could have a smoke test for this, to verify if we actually catch linting issues. However this would be out of scope and it would be fine to put this into a separate issues.

One request I have is to raise the same PR to the v8 branch so we can release it to the latest version.

@MadSandwich MadSandwich changed the title Flat eslint support Flat ESLint support Apr 18, 2024
@MadSandwich
Copy link
Contributor Author

@MadSandwich the change looks good to me. It would be awesome if we could have a smoke test for this, to verify if we actually catch linting issues. However this would be out of scope and it would be fine to put this into a separate issues.

One request I have is to raise the same PR to the v8 branch so we can release it to the latest version.

@christian-bromann I will try to write some smoke test on weekend. And same PR is raised on v8 (#12716)

@christian-bromann
Copy link
Member

@MadSandwich thanks, mind updating your branch? We have fixed some things in main.

@christian-bromann
Copy link
Member

@MadSandwich mhm .. I was hoping that your branch would pass now, mind taking a look?

@MadSandwich
Copy link
Contributor Author

@MadSandwich mhm .. I was hoping that your branch would pass now, mind taking a look?

@christian-bromann for no reason it fails build for wdio-global, and the only one thing help me successfully build is return back // @ts-expect-error this line, that you recently deleted.

@christian-bromann
Copy link
Member

@MadSandwich can you update your pnpm-lock file and ensure that expect-webdriverio is updated to v4.13.0?

@christian-bromann christian-bromann added the PR: New Feature 🚀 PRs that contain new features label Apr 22, 2024
Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

Thanks a ton!

@christian-bromann christian-bromann merged commit 6499f2b into webdriverio:main Apr 22, 2024
11 checks passed
@wdio-bot
Copy link
Contributor

Hey MadSandwich 👋

Thank you for your contribution to WebdriverIO! Your pull request has been marked as an "Expensable" contribution. We've sent you an email with further instructions on how to claim your expenses from our development fund. Please make sure to check your spam folder as well. If you have any questions, feel free to reach out to us at expense@webdriver.io or in the contributing channel on Discord.

We are looking forward to more contributions from you in the future 🙌

Have a nice day,
The WebdriverIO Team 🤖

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants