Skip to content

Feat: no-deprecated-api adheres to targeted node version #164

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

Merged
merged 3 commits into from
May 23, 2019
Merged

Feat: no-deprecated-api adheres to targeted node version #164

merged 3 commits into from
May 23, 2019

Conversation

Lalem001
Copy link
Contributor

@Lalem001 Lalem001 commented May 9, 2019

Patterned after no-unsupported-features

Resolves #141.

@Lalem001
Copy link
Contributor Author

Lalem001 commented May 9, 2019

@mysticatea, Few questions:

  • Should I update the no-deprecated-api.md?
  • Do you prefer PRs with a single commit (should squash)?
  • Should the default version be process.versions.node?

@mysticatea
Copy link
Owner

mysticatea commented May 10, 2019

Thank you for this PR.

But the current direction is different from I thought.

This should change only error messages to hide unusable alternatives. The change affects only how it shows replacedBy property.
For example, assert.equal has two alternative ways assert.strictEqual and assert.strict.equal, but the assert.strict.equal has been added in v9.9.0 so it should hide assert.strict.equal if the configured version range includes <9.9.0.


  • Should I update the no-deprecated-api.md?

No, I don't think this change needs document update. But updates are welcome because I'm poor at English :)

  • Do you prefer PRs with a single commit (should squash)?

No. Because I will use "squash and merge" button to merge PRs, I don't mind about the number of commits.

  • Should the default version be process.versions.node?

No. The runtime version is different from the versions that the program supports. Currently the default value is 8.0.0 because 8.x is the minimum version that the community is maintaining.

@Lalem001
Copy link
Contributor Author

Lalem001 commented May 10, 2019

@mysticatea I believe I understand what you are going for and I have updated the code. I hope you don't mind that I force pushed.

I also hope I got the correct supported versions for the different replacements.

As far as the no-deprecated-api.md docs are concerned, no-deprecated-api now takes version option. So I should probably at least document that.

@mysticatea mysticatea self-requested a review May 10, 2019 04:44
Copy link
Owner

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, thank you very much!
I have a small suggestion.

@Lalem001
Copy link
Contributor Author

@mysticatea Did you see my reply to your code comment?

@mysticatea
Copy link
Owner

I'm sorry for my delay.

...OK, fair enough. I will merge this PR later. Thank you very much!

@mysticatea mysticatea merged commit fd9d19d into mysticatea:master May 23, 2019
@mysticatea
Copy link
Owner

Thank you very much for your contribution!

@Lalem001 Lalem001 deleted the feature/no-deprecated-api-follows-node-ver branch May 23, 2019 14:20
@julien-f
Copy link

I'm not sure this is working, I have a project with is configured with:

"engines": {
  "node": ">=6"
}

But I still get:

  5:9  error  'url.parse' was deprecated since v11.0.0  node/no-deprecated-api

@julien-f
Copy link

Reproduction repository: https://github.com/julien-f/eslint-node-issue-141

@Lalem001
Copy link
Contributor Author

@julien-f in my opinion, it is working as intended. You specified that a node supported version range of greater than or equal to 6.0.0 which would include 11.0.0. Hence the notification that the api in question is deprecated since 11.0.0.

Further, Node recommends replacing ‘url.parse’ with the WHATWG URL API which is available as of 6.13.0.

@julien-f
Copy link

julien-f commented Sep 19, 2019

I disagree, it should not recommend an API that is not supported by a Node version compatible with my package (in this case URL available from Node 7).

This is exactly what is described in #141.

Edit: even if that's 6.13.0, it's not compatible for all the versions covered by >=6.

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.

The no-deprecated-api rule should adhere to the targeted Node.js version
3 participants