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

import/extensions doesn't work for require('./file-without-extension') #1230

Closed
balupton opened this issue Nov 13, 2018 · 6 comments · Fixed by #1237
Closed

import/extensions doesn't work for require('./file-without-extension') #1230

balupton opened this issue Nov 13, 2018 · 6 comments · Fixed by #1237

Comments

@balupton
Copy link

balupton commented Nov 13, 2018

See reproduction details at https://github.com/balupton/eslint-issue-import-extensions


get started:

git clone https://github.com/balupton/eslint-issue-import-extensions.git
cd eslint-issue-import-extensions
npm install

run the test:

npm test

notice that there was no complaint about:

const result = require('./module')

inside ./source/index.js

@balupton balupton changed the title import/extensions doens't seem to work import/extensions doesn't seem to work Nov 13, 2018
@balupton
Copy link
Author

balupton commented Nov 13, 2018

changing "sourceType": "script" to "sourceType": "module", and changing index.js to

'use strict'

import result from './module'

process.stdout.write(result)

produces the expected linting error about the extension. So seems this rule is only implemented for import statements rather than require statements too.

@balupton
Copy link
Author

balupton commented Nov 13, 2018

For the use case for why they should be for require statements too:

  • VS Code doesn't provide intellisense when the extension is omitted
  • TypeScript type checking (with the allowJs flag) doesn't follow paths when the extension is omitted

The result of the above is a any type, instead of the actual type.

When the extension is provided, the correct type information is achieved.

@balupton balupton changed the title import/extensions doesn't seem to work import/extensions doesn't work for require('./file-without-extension') Nov 13, 2018
@ljharb
Copy link
Member

ljharb commented Nov 13, 2018

The bullet points seem like a major flaw in both vscode and typescript, since omitting the extension is a best practice. Hopefully issues have been filed for those.

Separately, we should lint import and require statements both whenever possible, for every rule - so this is a good fix to make.

@balupton
Copy link
Author

Separately, we should lint import and require statements both whenever possible, for every rule - so this is a good fix to make.

agreed, thanks, looking forward to it

@ljqx
Copy link
Contributor

ljqx commented Nov 19, 2018

It seems better to have a helper to generate callbacks for ImportDeclaration/ExportNamedDeclaration/ExportAllDeclaration and static require CallExpression together?
Sth like following in rules:

module.exports = {
  create(context) {
    return addForGenericImport({
      ... // other callbacks
    }, (source, node) => {
      // callbacks for ImportDeclaration, static require CallExpression, ExportNamedDeclaration, ExportAllDeclaration, etc
    }),
  }
}

Actually most rules miss ExportNamedDeclaration/ExportAllDeclaration, and some rules miss static require CallExpression, not only import/extensions.

How do you think? @ljharb

@ljharb
Copy link
Member

ljharb commented Nov 19, 2018

A commonly used helper seems like a good idea.

ljharb pushed a commit to ljqx/eslint-plugin-import that referenced this issue Apr 12, 2019
ljqx added a commit to ljqx/eslint-plugin-import that referenced this issue Nov 17, 2020
ljqx added a commit to ljqx/eslint-plugin-import that referenced this issue Nov 18, 2020
ljqx added a commit to ljqx/eslint-plugin-import that referenced this issue Dec 11, 2020
ljharb pushed a commit to ljqx/eslint-plugin-import that referenced this issue Jan 22, 2021
@ljharb ljharb closed this as completed in 93e060f Jan 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants