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

Bug: prefer-node-protocol rule and deprecated builtins #337

Closed
1 task done
pascalduez opened this issue Sep 17, 2024 · 4 comments · Fixed by #338
Closed
1 task done

Bug: prefer-node-protocol rule and deprecated builtins #337

pascalduez opened this issue Sep 17, 2024 · 4 comments · Fixed by #338
Labels
accepted bug rule:update An update to a current rule

Comments

@pascalduez
Copy link

pascalduez commented Sep 17, 2024

Environment

Node version: 22.7.0
npm version: 10.8.2
ESLint version: 8.57.0
eslint-plugin-n version: 17.10.2
Operating System: Linux

What rule do you want to report?

prefer-node-protocol

Link to Minimal Reproducible Example

https://eslint-online-playground.netlify.app/#eNptkMFugzAMhl8l8qUXoFuPVNtpb7H0gIJB2YITOaFqhXj3OQ1om9SLY1uf//z2ApHNEW/dFBw2XxFasFPwnFSY6W58j2pgP6nDXh7OUAFGZyk1xtNgx39TVHC9IXVw82ipJg1nTZrw9qB6HLrZJfWpSaklB6UKGVu1iMhalSbPDnOrVEp06RgYB+SaxEwd2CdvvNPQihggs2cN1e9AHrkiR+vFQivF+9vp1Lw2L0LtzHop2ZofCRexKkuGznx3Yz6KJ1nwIakh3QMWpcn34m7T0dDj9QMDUo9kLMbMbC72Y5Qx1yWMkm/fP7nUX2qztML6A41SiaQ=

What did you expect to happen?

The prefer-node-protocol is reporting the import of punycode, as missing the node: prefix, which is intended. The issue is the punycode Node builtin is deprecated and we are encouraged to use a third party module instead.
Which we do in our codebase currently. We use a module called punycode, so we don't want to prefix it obviously.

See https://nodejs.org/api/punycode.html#punycode

So it would be a good thing to maybe have another option to the rule to be able to ignore certain Node builtins?

"n/prefer-node-protocol": ["error", {
  "ignore": ['punycode'],
}]

Alternatively the plugin could maintain a list of deprecated builtins related to the Node version and exclude them, but that sound quite tedious and not optimal. Also not working with ESM.

We should note that we can work around this issue with import punycode from 'punycode/' but is that's not realy satisfactory.

Participation

  • I am willing to submit a pull request for this issue.

Additional comments

No response

@pascalduez pascalduez added the bug label Sep 17, 2024
@scagood
Copy link

scagood commented Sep 17, 2024

Thank you for the report!

This should have been covered by https://github.com/eslint-community/eslint-plugin-n/blob/master/lib/rules/prefer-node-protocol.js#L154

It seems that node:punycode is detected as a real module!
image

scagood added a commit to scagood/eslint-plugin-n that referenced this issue Sep 17, 2024
@scagood scagood added accepted rule:update An update to a current rule labels Sep 17, 2024
@scagood
Copy link

scagood commented Sep 17, 2024

@pascalduez One thing I forgot to mention is that if you're importing:

const punycode = require('punycode');

Then you're going to be importing the wrong module! You need to import punycode like this:

const punycode = require('punycode/');

aladdin-add pushed a commit that referenced this issue Sep 18, 2024
* test: Add failing test for #337

* fix: Use our data set to work out if a module is a node module
@pascalduez
Copy link
Author

@pascalduez One thing I forgot to mention is that if you're importing:

const punycode = require('punycode');

Then you're going to be importing the wrong module! You need to import punycode like this:

const punycode = require('punycode/');

Yes thank you, I was aware of this workaround, but it does not work with ES modules. And honestly, feels a bit hacky.

But as you mentioned this issue made us realize that we are using different modules server side (the Node buitin) and client side (the npm package) which is worrying because they are supposed to have slight differences.

At the end we are going to switch to punycode.js (same package) to remove any ambiguities.

Thanks for the very quick fix!

@scagood
Copy link

scagood commented Sep 18, 2024

At the end we are going to switch to punycode.js (same package) to remove any ambiguities.

I think it may be better to suggest punycode.js instead of punycode here too:
https://github.com/eslint-community/eslint-plugin-n/blob/master/lib/rules/no-deprecated-api.js#L334-L339

I was unaware of that package as a mirror of the punycode repo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted bug rule:update An update to a current rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants