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

n/file-extension-in-import triggered incorrectly on some modules with / in path. #21

Closed
rosskevin opened this issue Jun 3, 2022 · 7 comments

Comments

@rosskevin
Copy link

require file extension '.js' n/file-extension-in-import error is being triggered incorrectly for a few modules including @apollo/client/core and yargs/helpers

The key driver to these appear to be the occurrence of a slash even, which may be tripping a condition to call it a local file.

I am investigating now to see if I can solve and PR.

@rosskevin
Copy link
Author

The regex for package is not working, see https://regex101.com/r/Dch2Gv/1

Specifically, there is a basic regex test for a valid package to get out early packageNamePattern.test(name) where the name is either @apollo/client/core or yargs/helpers.

This may have to get smarter by examining the exports in the package.json or some other way, this could be very painful if there is not another utility that provides resolution.

I'll look at the import plugin and see what they are doing.

@rosskevin
Copy link
Author

rosskevin commented Jun 3, 2022

So this is a confirmed bug, and I have looked at eslint-plugin-import for ideas. I attempted to bring some code over, but it started spidering into a ton of code. It doesn't help that this repo is still authored in CJS and that one is more modern ECMA. They are building up more contextual information about the package, which is ultimately going to be necessary for proper resolution of modules.

eslint-plugin-import's import/extensions rule has 1. no fixer, 2. no typescript support.

Ultimately, I think one of the following needs to happen:

  1. the differential functionality needs to get PR'd there (fixer plus typescript support)
  2. big effort to duplicate code (or get eslint-plugin-import to extract more to a reusable module - they already have some in eslint-module-utils) then fix here.
  3. Play dumb and incorporate something like an ignorePaths for a simple opt-out on extension replacement.

The problem with PR'ing to the eslint-plugin-import repo, is that it looks to be falling behind due to the scope (78 PRs, 432 issues). I'm not confident that PR'ing will get accepted in any timeframe. On the flip side, the eslint-plugin-import does seem well written.

Any of the above are possible, but I'm afraid I've got almost a full day into this so I have to pull the ripcord and move on.

Thoughts?

@aladdin-add
Copy link

aladdin-add commented Jun 6, 2022

if it's a local module (./xxx) or import maps, the ext is always required. In other cases, does it help to just use lib/converted-esm/import-meta-resolve.js( added in d24be36) to resolve the module?

@rosskevin
Copy link
Author

That may be of some help, but on cursory look I cannot tell if it's going to be enough. I'm sorry to abandon this issue at this point, but I have to move on to other priorities. I hope my research has documented the issue and someone else will chip in some effort.

@jimmywarting
Copy link

shouldn't it just check if it starts with a . first and formost? so that it only operates on local files and not on some nodejs or npm module

@vajahath
Copy link

I've similar issue with firebase packages. Asking me to put extensions:
Screenshot from 2023-02-28 21-10-54

image

prisis added a commit to anolilab/javascript-style-guide that referenced this issue Jul 28, 2023
The 'n/file-extension-in-import' rule has been removed from the 'overrides' section and configured as 'off'. This was done due to the rule being buggy as reported in the issue eslint-community/eslint-plugin-n#21. Some code comments were also added to describe certain rules. The purpose of this change is to prevent undesired ESLint warnings.

Signed-off-by: prisis <d.bannert@anolilab.de>
@aladdin-add
Copy link

fixed by #132

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 a pull request may close this issue.

4 participants