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

Rule Change: no-restricted-imports should honor a package's declared exports #18104

Closed
1 task done
JeremyLoy opened this issue Feb 9, 2024 · 6 comments
Closed
1 task done
Labels
enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules

Comments

@JeremyLoy
Copy link

JeremyLoy commented Feb 9, 2024

What rule do you want to change?

no-restricted-imports, although I'm open to any rule set to be honest

What change do you want to make?

Generate more warnings

How do you think the change should be implemented?

A new default behavior

Example code

https://github.com/JeremyLoy/hidden-module-import-issue-example/blob/main/test/index.test.mjs

I made a sample project demonstrating the issue.

https://nodejs.org/api/packages.html#package-entry-points

Since Node v12 it has been possible for a package to define it's exports, encapsulating unlisted modules. This is a great way to write scalable code.

Unfortunately, importing this hidden modules doesn't cause an issue until runtime. This means that it is up to Linters and IDEs to raise the issue to developers early.

I've opened an issue for Webstorm already, but I think ESLint would be a great place as well.


Because this has been the default behavior for NodeJS for many years, I think its safe to add this new rule behavior and default it to 'on' as well.

What does the rule currently do for this code?

It does nothing for the attached sample project.

What will the rule do after it's changed?

It will warn against import violations defined by packages in the exact same way as manually defined import violations

Participation

  • I am willing to submit a pull request to implement this change.

Additional comments

Happy to help if you point me in the correct direction. I've never contributed to this codebase specifically nor have I ever looked at its internals.

@JeremyLoy JeremyLoy added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules labels Feb 9, 2024
@JeremyLoy JeremyLoy changed the title Rule Change: no-restricted-imports should honor a packages declared exports Rule Change: no-restricted-imports should honor a package's declared exports Feb 9, 2024
@fasttime
Copy link
Member

Thanks for the issue @JeremyLoy. I don't think that ESLint can support this feature in a core rule because it's about a Node.js-specific API. Also note that the no-restricted-imports rule is designed to work without knowledge of the file system. It even works in a browser because it expects a list of import specifiers to be provided by the user, without checking if those imports correspond to existing or accessible modules.

That said, eslint-plugin-n has a rule called n/no-missing-import that should report a problem in this case (I tried it with your repro and it does, although there's another plugin in your config that doesn't recognize the subpath export and reports a problem).

@JeremyLoy
Copy link
Author

Thanks for the response @fasttime, I appreciate it. I'll close this since it makes sense to not have Node only concepts in ESLint proper.

Also, thank you for pointing me towards eslint-plugin-n, I hadn't seen it before.

Is that the de-facto plugin for all things NodeJS specific nowadays? I see it referenced a few times in this repo next to deprecated rules

@JeremyLoy JeremyLoy closed this as not planned Won't fix, can't repro, duplicate, stale Feb 10, 2024
@JeremyLoy
Copy link
Author

hey @fasttime - I updated my repo with eslint-plugin-n, but I'm still not seeing the lint error like you said.

Do you mind taking a look and seeing what I have misconfigured?

@fasttime
Copy link
Member

Hm, your npm "lint" script in the package.json file looks incomplete if you are running npm run lint. It should be along the lines of "eslint --ext .mjs .".

In the eslintrc config system, no files are linted by default, and only .js files are linted if a directory is specified. To lint a directory of .mjs files, one would pass both a directory name (. for the current directory) and the file extension with --ext .mjs on the command line. When using the flat config system, which will become the default in ESLint 9, neither is required.

@JeremyLoy
Copy link
Author

@fasttime thanks 🤦 Can't believe I forgot to do that. Its been a while since I set up a project from scratch.

My PoC is working now, correctly failing the impossible import. But now its also failing to find the local workspace module. Unless you think thats an issue with eslint specifically, I'll start a conversation over at eslint-plugin-n.

Thanks again for your help!

@fasttime
Copy link
Member

fasttime commented Feb 13, 2024

No problem :) For usage questions around ESLint there is also a Discord chat, they are usually quick to respond. Good luck!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
Archived in project
Development

No branches or pull requests

2 participants