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

16.6.2: can n/no-missing-require allow resolvePaths ['./'] for relative imports? #210

Open
1 task
lkraav opened this issue Apr 1, 2024 · 16 comments
Open
1 task

Comments

@lkraav
Copy link

lkraav commented Apr 1, 2024

Environment

Node version: 16.20.2
npm version: 8
ESLint version: 8.57
eslint-plugin-n version: 16.6.2
Operating System: Linux

What rule do you want to report?

n/no-missing-require

Link to Minimal Reproducible Example

What did you expect to happen?

const config = require('config'); is getting flagged for n/no-missing-require, even though config/ module directory is living next to the source file.

(I'll try following up with a stackblitz example if this is unclear.)

Participation

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

Additional comments

Am I missing something obvious for why below doesn't help

  rules: {
    'n/no-missing-require': ['error', {
      'resolvePaths': ['./'],
    }],
  },

I'm not against refactoring everything to explicit require('./config'), but would like to allow sibling requires until we get it done. Is this possible?

PS not a "bug" I think, but Discussions isn't enabled on this repo.

@lkraav lkraav added the bug label Apr 1, 2024
@scagood scagood added enhancement and removed bug labels Apr 1, 2024
@scagood
Copy link

scagood commented Apr 1, 2024

Where would it resolve the ./ from? do you think it should be resolved from the eslint cwd options?

By the time the plugin gets the setting we dont really know where the config file was 🤔

@lkraav
Copy link
Author

lkraav commented Apr 1, 2024

Where would it resolve the ./ from? do you think it should be resolved from the eslint cwd options?

By the time the plugin gets the setting we dont really know where the config file was 🤔

Yes, I had the same question. In my naive mind, relative resolvePaths would resolve in a JIT manner, so calculated on each specific target file.

But maybe this is a bug, after all, and subject to codebase update? Maybe n/no-missing-require should simply not trigger on requiring any file's sibling modules? Because everything else seems to recognize such as valid, or at the very least, definitey not "not found".

@scagood
Copy link

scagood commented Apr 1, 2024

Can you provide a small example of where this applies?

I have setup a small starting point for you here: https://eslint-online-playground.netlify.app/#eNpNkL1 Once you have the error there, can you paste the url here?

@scagood
Copy link

scagood commented Apr 1, 2024

I am not sure if that is a valid resolution?

Are you using a special library that resolves these?

I would expect that resolution to be written like: ./config, ./config/index, or ./config/index js

@lkraav
Copy link
Author

lkraav commented Apr 1, 2024

We have jsconfig.json files involved, I wonder if these are having an actual effect here

{
  "compilerOptions": {
    "baseUrl": "./src/",
    "module": "nodenext",
    "paths": {
      "*": ["./*"],
      "lib/*": ["../../../packages/lib/server/*"]
    }
  }
}

That first wildcard may be the one doing the trick? 🤔

@scagood
Copy link

scagood commented Apr 2, 2024

mmm, so this is a request to support jsconfig.json, like we do for the tsconfig.json

https://github.com/eslint-community/eslint-plugin-n/blob/master/lib/util/import-target.js#L272-L275

@lkraav
Copy link
Author

lkraav commented Apr 2, 2024

mmm, so this is a request to support jsconfig.json, like we do for the tsconfig.json

Booyah, you are probably right. Is this a fairly light lift to try a test PR, considering tsconfig support already exists, and afaik they're fairly similar?

@aladdin-add
Copy link

just a reminder: you can use tsconfig.json + "allowJs": true. :)

@lkraav
Copy link
Author

lkraav commented Apr 2, 2024

just a reminder: you can use tsconfig.json + "allowJs": true. :)

Interesting idea. Is this a 100% drop-in change, even with no other TS tooling installed yet?

TS is on the roadmap in the coming month, so I wouldn't be opposed to making this rename move early, if it's without side-effects 🤔

@scagood
Copy link

scagood commented Apr 2, 2024

It looks to be, but I am not completely sure 👀

https://code.visualstudio.com/docs/languages/jsconfig

@lkraav
Copy link
Author

lkraav commented Apr 2, 2024

It looks to be, but I am not completely sure 👀

Tip: jsconfig.json is a descendant of tsconfig.json, which is a configuration file for TypeScript. jsconfig.json is tsconfig.json with "allowJs" attribute set to true.

Thanks for reminding me of the docs link, above paragraph definitely looks tasty enough to try right away.

@lkraav
Copy link
Author

lkraav commented Apr 4, 2024

I don't see going to tsconfig.json having helped

{
  "compilerOptions": {
    "allowJs": true,
    "baseUrl": "./src/",
    "module": "nodenext",
    "paths": {
      "*": ["./*"],
      "lib/*": ["../../../packages/lib/server/*"]
    }
  }
}

still gives me

[leho:project.git] demo(+0/-8,3)+* ± eslint apps/api/src/resources/test/test.service.js

/home/leho/Documents/project.git/apps/api/src/resources/test/test.service.js
    1:24  error  "config" is not found                                                 n/no-missing-require

@lkraav
Copy link
Author

lkraav commented Apr 4, 2024

And another roadblock: #139 for TS only appeared in v17, but we're still on Node 16, so stuck on v16 right now. PR seemed fairly large, is there even a chance of it applying for a backport?

@aladdin-add
Copy link

It's not possible as it's a breaking change. and we will release v17(latest) soon then the v16 is going to be EOL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants