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

Update for eslint v8 #29

Merged
merged 1 commit into from Oct 30, 2021
Merged

Conversation

klassm
Copy link
Contributor

@klassm klassm commented Oct 28, 2021

Changes for making the eslint rule compatible with eslint 8. In general this is about setting hasSuggestion to true for suggestion rules.
In your case it looks like the rule is actually not a suggestion. More details at the respective line.

@klassm klassm force-pushed the eslint8 branch 2 times, most recently from 5481a30 to 6891e41 Compare October 28, 2021 12:17
@@ -47,6 +47,7 @@ module.exports = {
{ devDependencies: true }
],
"no-console": "off",
"@typescript-eslint/no-unsafe-argument": "off",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for the e2e test - jest objectContaining will return an any. I did not find a way around it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

See the example.serial-test.ts changes in commit [7840c91] of the release/v2.0 branch, I used a type-cast since that is the only way to override jest's poor decision to explicitly return any rather than a generic.

https://github.com/testcafe-community/eslint-plugin-testcafe-community/blob/release/v2.0/tests/example.serial-test.ts

@@ -17,11 +17,10 @@ export default createRule({
messages: {
noSkip: "Do not use the `.skip` hook."
},
type: "suggestion",
type: "problem",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A suggestion needs to also provide a suggestion array - at least if I understood the docs correctly.
This is actually also the underlying error - if you have a suggestion, you need to set hasSuggestion also to true. But if you do that the require-meta-has-suggestions rule will break.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I did not run into this error about suggestions when I worked on the update to eslint@v8. Will this be a breaking change for v7?

@codejedi365
Copy link
Collaborator

codejedi365 commented Oct 28, 2021

@klassm, Good work and good effort trying to resolve everything for an update. Surprisingly, I attempted the same thing this past weekend as part of the release/v2.0 branch. I did not run into an issue related to a suggestion rule. Could you provide some insight how you found that?

I did find a couple of issues that held up the expansion to support eslint@v8.

  1. eslint-config-airbnb-base#2478 is not ready for eslint@v8 which means I do not want to bump all the devDependencies yet since it is for developers use, BUT in terms of the production plugin when used by others, I want to support v7.0 || v8.0.

  2. I also found the plugin is actually not compatible with v6.8 even though the peer dependency says it is. Not sure when that broke, It might just be for v2.0 refactor but likely not. Given I'm restricting compatibility, I feel it should be a part of a major version update.

I do appreciate the effort you put forth. I think I will need to think a bit more about what is the best method to release your PR and the v2.0 features. I hoped to release v2.0 this past weekend but I kept hitting snags and so there is still stuff to work through. You might be able to rebase or branch from release/v2.0 (I think it's clean enough) and help transition to eslint@v8.

With that said, I will be unavailable to work through next week meaning I would anticipate a release date around Nov 13th.

@klassm
Copy link
Contributor Author

klassm commented Oct 28, 2021

Hey hey. Sure, you also put a lot of thinking into this already :-). Maybe the info why I changed the plugin - probably I noted this because I pulled everything to eslint 8. This meant that also the eslint configuration for the project is suddenly eslint 8. When I did that eslint itself started to complain that the rule is actually not a suggestion role. You could also add the hasSuggestion flag - but it did not help, then it complained about something else. In the end I found that the rule itself does not really provide any suggestions, does it? So probably problem is more fitting.

I did not totally get your comment about airbnb. When I pulled up everything to v8, I also pulled up all the plugins. The project compiled just fine :-)

@codejedi365
Copy link
Collaborator

Not sure how familiar you are with dependency types but plugins usually use the peerDependencies category. DevDependencies define packages only used for development purposes which is why they are not installed in a production install (a install as a dependency for another project).

In relation to AirBnB's configuration as a plugin, they have a set peerDependency:

{
  "peerDependencies": {
      "eslint": "^5.0.0 || ^6.8.0 || ^7.32.0"
  }
}

There is an open issue for an update. Since I use eslint-plugin-airbnb-typescript and eslint-plugin-airbnb-base they require the dependency of eslint@^7. You bumped this package's devDependency to eslint@^8.1.0. If you run my dev run-script npm run lint, AirBnb will not work. This is key for maintaining a familiar codebase for PRs and such.

What I combined into the same statement was my desire to support eslint@v8 as a listed entry in the peerDependencies of eslint-plugin-testcafe-community. AirBnB not making an update to support v8 is not a limiting factor for testcafe-community plugin to update to support v8.

Ultimately, what I did in release/v2.0 was only update testcafe-community to support eslint@v8 through manual testing in the example package but not actually bump any devDependencies. I only added to the peerDependency listing:

{
  "peerDependencies": {
      "eslint": "^7 || ^8"
  }
}

Copy link
Collaborator

@codejedi365 codejedi365 left a comment

Choose a reason for hiding this comment

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

I merged this branch into a working branch with some improvements off of next. Made the changes we discussed and luckily overcame any issues with TS compilation.

This is now in version v1.3.0 of the eslint-plugin-testcafe-community@next package.

@@ -47,6 +47,7 @@ module.exports = {
{ devDependencies: true }
],
"no-console": "off",
"@typescript-eslint/no-unsafe-argument": "off",
Copy link
Collaborator

Choose a reason for hiding this comment

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

See the example.serial-test.ts changes in commit [7840c91] of the release/v2.0 branch, I used a type-cast since that is the only way to override jest's poor decision to explicitly return any rather than a generic.

https://github.com/testcafe-community/eslint-plugin-testcafe-community/blob/release/v2.0/tests/example.serial-test.ts

@@ -17,11 +17,10 @@ export default createRule({
messages: {
noSkip: "Do not use the `.skip` hook."
},
type: "suggestion",
type: "problem",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I did not run into this error about suggestions when I worked on the update to eslint@v8. Will this be a breaking change for v7?

"@typescript-eslint/eslint-plugin": "^5.2.0",
"@typescript-eslint/parser": "^5.2.0",
"@typescript-eslint/types": "^5.2.0",
"eslint": "^8.1.0",
"eslint-config-airbnb-base": "^14.2.1",
"eslint-config-airbnb-typescript": "^14.0.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not ready yet to make the switch to v8 within the devDependencies. eslint-config-airbnb is not yet compatible with v8.

@codejedi365 codejedi365 merged commit 8717cc3 into testcafe-community:master Oct 30, 2021
@github-actions
Copy link

🎉 This PR is included in version 1.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

2 participants