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

feat(deps): update deps, support of eslint 8 #38

Closed
wants to merge 7 commits into from

Conversation

Delagen
Copy link
Collaborator

@Delagen Delagen commented Oct 12, 2021

Not sure if it correct, but who doesn't make errors?

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 26847 lines exceeds the maximum allowed for the inline comments feature.

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 26883 lines exceeds the maximum allowed for the inline comments feature.

@d-arrington
Copy link

For what it's worth, I pulled your branch into my current project and it seems to be working as intended.

Copy link

@bfaulk96 bfaulk96 left a comment

Choose a reason for hiding this comment

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

LGTM

@crystalfp
Copy link

Please merge! It is the last eslint plugin that does not work under eslint 8. Thanks!

@paulius-valiunas
Copy link

I know this is not even merged yet, but what's the release schedule here? Can we expect this change to be part of a release in the following days?

@bfaulk96
Copy link

Well considering there hasn't been a release for this in 5 months... I wouldn't count on it happening too quickly haha

@paulius-valiunas
Copy link

I mean it's understandable since there haven't been any merged PRs since May. Last time there was a PR that was merged, a new patch version was pushed out on the same day. If only there was a way to get @gund's attention... 😊

@bfaulk96
Copy link

@gund It's totally understandable that you are probably quite busy, but is there any chance you'd be open to people being added as maintainers so that this project isn't left untouched for long periods of time?

@jdom
Copy link

jdom commented Nov 1, 2021

+1 I'm currently blocked from updating eslint due to this plugin, which blocks us from updating @typescript-eslint packages, which blocks us from updating typescript itself now (as TypeScript 4.4 isn't supported by older eslint packages)!
EDIT: I misjudged, I was able to update the @typescript-eslint packages to version 5.2, and that didn't require updating eslint to version 8 (I was at least able to update to 7.32, not sure if that's required).

@bfaulk96
Copy link

bfaulk96 commented Nov 1, 2021

Kind of sounds like it might be time for us to ask npm to declare it up for adoption and fork it? Since none of the contributors seem to be active or responsive 🙁
Or we could just fork it and publish it as eslint-plugin-deprecations (with an s) 😅

@jleider
Copy link

jleider commented Nov 8, 2021

While we are waiting for these things to be merged I created a quick and dirty release with this change and #33.

Just add the following to your package.json:

"eslint-plugin-deprecation": "https://github.com/jleider/eslint-plugin-deprecation#v1.3.0"

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 26961 lines exceeds the maximum allowed for the inline comments feature.

@Delagen
Copy link
Collaborator Author

Delagen commented Nov 9, 2021

Published as @delagen/eslint-plugin-deprecation, also included #35

@lachieh
Copy link

lachieh commented Nov 9, 2021

Rather than continuing to fork and publish new versions, perhaps the typescript-eslint team would be interested in bringing in this rule.

@JamesHenry is there a process for this sort of thing? Is that something the typescript-eslint team would do?

Thread TL;DR:
This rule doesn't yet support Eslint 8 and the maintainer @gund has been radio silent for the last 6 months.

@jdom
Copy link

jdom commented Nov 9, 2021

I updated my previous comment, looks like I'm no longer blocked.

I misjudged, I was able to update the @typescript-eslint packages to version 5.2, and that didn't require updating eslint to version 8 (I was at least able to update to 7.32, not sure if that's required).

Having said that disclaimer, I'd still like the package to support to support eslint 8, so that we can move forward. Alternatively, it would be awesome to have this rule officially supported by the TypeScript team as @lachieh suggested

@crystalfp
Copy link

Thanks a lot @Delagen for your work!
I need just an update on how to use the updated plugin.

  1. Installed with npm i -D @delagen/eslint-plugin-deprecation
  2. In eslint.yaml added - "@delagen/eslint-plugin-deprecation" under plugins
  3. Under rules: I tried:
    - "@delagen/eslint-plugin-deprecation: warn"
    - "@delagen/eslint-plugin-deprecation/deprecation: warn"
    - deprecation/deprecation: warn (as was before)

But always eslint responds with Definition for rule 'deprecation' was not found (deprecation)

Could you help me?

@Delagen
Copy link
Collaborator Author

Delagen commented Nov 9, 2021

@crystalfp just installed it to try to investigate problem. Seems Eslint support for namespaced plugins is tweaky

@Delagen
Copy link
Collaborator Author

Delagen commented Nov 9, 2021

@crystalfp Seems working

plugins:[
"@delagen/deprecation"
],
rules:{
"@delagen/deprecation/deprecation": "warn"
}

What version of ESLINT are you using?

@crystalfp
Copy link

With your suggestion it works! Now I have to think of a deprecation just to test.
I'm using eslint 8.0.0
Thanks!
mario

@@ -1,12 +1,18 @@
{
"name": "eslint-plugin-deprecation",
"version": "0.0.0-development",
"name": "@delagen/eslint-plugin-deprecation",

Choose a reason for hiding this comment

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

Looks like some of the changes include in this file might not have been intended for the upstream update?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is because of fork. I updated this project in master branch

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@timgates42 if you has permission to merge and publish npm, I can rebase this Mr

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

truncated into #39

@lachieh
Copy link

lachieh commented Dec 3, 2021

It looks like there has been an issue open for a while to add a deprecation rule into the typescript-eslint project.

That same issue references another issue that talks about one of three options:

If someone wants to port the rule to an independent eslint plugin, feel free!
If someone wants to simply document how to use the sonar rule, they can do that instead.
If someone wants to independently implement the rule based on tslint's implementation, they can do that too.

Originally posted by @bradzacher in typescript-eslint/typescript-eslint#1223 (comment)

It looks like @gund went down the first route, but given the lack of activity, it might be about time to take a look at implementing the third (implementing a rule based on the tslint implemention).

I'll likely have a look at this path over the weekend, but if anyone else wants to take a look, feel free.

@bfaulk96
Copy link

bfaulk96 commented Dec 3, 2021

@lachieh It's worth noting that while I fully agree this would be great to have in typescript-eslint, @Delagen forked this plugin and fixed the problems (for option 1), but the issue now is that this plugin is taking up dead space in npm.

"importHelpers": true,
"sourceMap": true,
"module": "CommonJS",
"target": "ES2015",
Copy link

Choose a reason for hiding this comment

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

Not sure if relevant but the change from ES5 to ES2015 is a breaking change :)

ES5 is not ES2015. ES2015 is ES6.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you see supported versions of node.js by eslint, I think it can be even higher

Copy link

Choose a reason for hiding this comment

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

You're right. I just saw the target change and figured I'd point it out :)

But given that ESLint 8 only supports "node": "^12.22.0 || ^14.17.0 || >=16.0.0" then I guess it's safe to use ES2019
https://node.green/#ES2019

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 26977 lines exceeds the maximum allowed for the inline comments feature.

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 27134 lines exceeds the maximum allowed for the inline comments feature.

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 27164 lines exceeds the maximum allowed for the inline comments feature.

@codeclimate
Copy link

codeclimate bot commented Dec 9, 2021

Code Climate has analyzed commit 1893daf and detected 0 issues on this pull request.

View more on Code Climate.

@gund
Copy link
Owner

gund commented Dec 14, 2021

Hey folks, this has been resolved in #39!

@gund gund closed this Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.