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: eslint-plugin-node to eslint-plugin-n #26907

Merged
merged 2 commits into from Mar 18, 2024

Conversation

aladdin-add
Copy link
Contributor

@aladdin-add aladdin-add commented Jan 29, 2024

it was forked from eslint-plugin-node v11.1.0. as the original repository seems no longer maintained.

repo link: github.com/eslint-community/eslint-plugin-n
npm link: https://www.npmjs.com/package/eslint-plugin-n
related discussion: mysticatea/eslint-plugin-node#300

Changes

Context

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@CLAassistant
Copy link

CLAassistant commented Jan 29, 2024

CLA assistant check
All committers have signed the CLA.

@rarkins
Copy link
Collaborator

rarkins commented Jan 29, 2024

Please provide some links/references

@aladdin-add
Copy link
Contributor Author

yes, it's just a wip. will provide more info and sign the cla later.

@voxpelli
Copy link

I guess one challenge here is that eg the eslint rules are renamed from node/ to n/ and the plugins and/or extends of an eslint config also needs to reflect the new name

Can this be handled?

@viceice
Copy link
Member

viceice commented Jan 29, 2024

I guess one challenge here is that eg the eslint rules are renamed from node/ to n/ and the plugins and/or extends of an eslint config also needs to reflect the new name

Can this be handled?

no

@voxpelli
Copy link

no

Then sadly I think this isn't possible to do right now.

Would be nice if one could eg. show a replacement suggestion in the Dashboard issue, like:

### Unmaintained dependencies with community replacements:

* `eslint-plugin-node` has been superseded by `eslint-plugin-n`

@viceice
Copy link
Member

viceice commented Jan 30, 2024

renovate should simply open the replacement PR and let a maintainer fix the migration needed.

Copy link

@voxpelli voxpelli left a comment

Choose a reason for hiding this comment

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

The name got a bit mixed up with npm-run-all 😜

lib/config/presets/internal/replacements.ts Outdated Show resolved Hide resolved
lib/config/presets/internal/replacements.ts Outdated Show resolved Hide resolved
lib/config/presets/internal/replacements.ts Outdated Show resolved Hide resolved
@voxpelli
Copy link

renovate should simply open the replacement PR and let a maintainer fix the migration needed.

@viceice That assumes that people's tests will fail so that they realize that they need to do additional manual changes

To make matters even worse: If they are using a shared ESLint config, then it might not even be they who should make the change, it should be the upstream

@aladdin-add aladdin-add force-pushed the feat/npm-plugin-node-fork branch 2 times, most recently from 860508e to 7efcf5a Compare January 30, 2024 09:49
@aladdin-add aladdin-add marked this pull request as ready for review January 30, 2024 10:06
@aladdin-add
Copy link
Contributor Author

updated the OP, and signed the CLA.🎉

@aladdin-add
Copy link
Contributor Author

That assumes that people's tests will fail so that they realize that they need to do additional manual changes

Can you give an example of a situation where a ci won't fail?🤔

@voxpelli
Copy link

That assumes that people's tests will fail so that they realize that they need to do additional manual changes

Can you give an example of a situation where a ci won't fail?🤔

When there is no CI :) Not everyone runs their ESLint on CI

@viceice
Copy link
Member

viceice commented Jan 30, 2024

That assumes that people's tests will fail so that they realize that they need to do additional manual changes

Can you give an example of a situation where a ci won't fail?🤔

When there is no CI :) Not everyone runs their ESLint on CI

that's not renovate's fault. renovate won't automerge r placements by default, so it's the developer who is responsible to check required changes before merging such PRs.

@voxpelli
Copy link

Lets try it then and see what the feedback is, would be great to get people moving to it 👍

viceice
viceice previously approved these changes Jan 31, 2024
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

please don't force-push in future, we're doing squash merge

@voxpelli
Copy link

voxpelli commented Feb 5, 2024

@aladdin-add Can you maybe update the title of this PR to refer to eslint- instead of npm- as well? Then eg. a straight up squash commit will have the correct wording

@aladdin-add aladdin-add changed the title feat: npm-plugin-node to npm-plugin-n feat: eslint-plugin-node to eslint-plugin-n Feb 5, 2024
@aladdin-add
Copy link
Contributor Author

my bad! 😅 updated~

Copy link
Collaborator

@HonkingGoose HonkingGoose left a comment

Choose a reason for hiding this comment

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

Small style change to the preset name maybe?

lib/config/presets/internal/replacements.ts Outdated Show resolved Hide resolved
lib/config/presets/internal/replacements.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@HonkingGoose HonkingGoose left a comment

Choose a reason for hiding this comment

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

This approval is only for the text content. 😉

I'll leave it to the maintainers to approve this preset, and to review if this fork is the best replacement.

@voxpelli
Copy link

voxpelli commented Feb 7, 2024

if this fork is the best replacement

Some added context here that may be helpful for them:

This is the fork that's maintained by the official ESLint Community – a home for high-value projects in the ESLint ecosystem where the ESLint community collectively can ensure that they continue to be maintained instead of eg. relying on any single maintainer / releaser.

@aladdin-add
Copy link
Contributor Author

friendly ping...

@rarkins
Copy link
Collaborator

rarkins commented Mar 4, 2024

So I think we confirmed:

  • Old package is unmaintained
  • New package is legitimate

But is there a problem if this PR breaks people's scripts due to migration being necessary?

viceice
viceice previously approved these changes Mar 4, 2024
@viceice
Copy link
Member

viceice commented Mar 4, 2024

I think we should limit current version to ^11.1.0, so only if a user is on latest eslint-plugin-node major then open a replacement PR? Otherwise we suggest updates to older versions too, even if the user disabled majors.

viceice
viceice previously approved these changes Mar 4, 2024
@viceice
Copy link
Member

viceice commented Mar 4, 2024

still conflicted

@aladdin-add
Copy link
Contributor Author

still conflicted

should be addressed now.

@rarkins rarkins requested a review from viceice March 18, 2024 21:42
@rarkins rarkins added this pull request to the merge queue Mar 18, 2024
Merged via the queue into renovatebot:main with commit af31adb Mar 18, 2024
37 checks passed
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 37.256.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@aladdin-add aladdin-add deleted the feat/npm-plugin-node-fork branch March 19, 2024 02:05
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants