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

"no-named-as-default" should be changed to lessen false positives from 3rd party modules #123

Closed
Zamiell opened this issue Aug 4, 2024 · 7 comments · Fixed by #137
Closed

Comments

@Zamiell
Copy link

Zamiell commented Aug 4, 2024

The "no-named-as-default" rule throws some false positives.

Consider an error on the following line of code, which invokes the popular klaw-sync library:

import klawSync from "klaw-sync";

This errors because the module is not smart enough to figure out that variable names cannot have hyphens.

The following is a similar error from TypeScript:

import ts from "typescript";

While the "no-named-as-default" rule does seem to provide value to a codebase, it is undesirable to have to add "eslint-disable-line" entries across codebases whenever 3rd-party libraries need to be invoked like this. I feel that 3rd party libraries of this nature are common enough such that many users will disable the rule altogether, which is an unfortunate outcome.

Can we add an option to the rule to ignore all 3rd party libraries (from e.g. "node_modules" or whatnot)? Or is there some other good way to reduce this kind of false positive?

Conceptually, this lint rule is about preventing bugs from creeping in to the codebase when files with default exports are refactored/renamed. Subsequently, I do not think that this class of error applies to a 3rd party modules, since the default name correlates directly to the name of the dependency that is tied to the "package.json" file. Subsequently, I would argue that ignoring this kind of thing should be the default behavior.

@controversial
Copy link

I have a lot of false positives for this rule related, I think, to import-js#1594

The issue with a lot of pacakges is that they export the same value as a named import and as a default import:

export const z = /* ... */
export default z

This leads to errors for common imports like:

import React from 'react';
import z from 'zod';
import classNames from 'classnames';

It would be great if there could be a way for this rule to detect cases where the default export refers to the same value as the named export, and avoid reporting the error in this special case

SukkaW added a commit to SukkaW/eslint-plugin-import-x that referenced this issue Aug 26, 2024
SukkaW added a commit to SukkaW/eslint-plugin-import-x that referenced this issue Aug 26, 2024
@SukkaW
Copy link
Collaborator

SukkaW commented Aug 26, 2024

import z from 'zod';

@controversial This fails because you can do both:

import z from 'zod';
import { z } from 'zod';

And the rule no-named-as-default prefers the later.

@controversial
Copy link

@SukkaW Thanks for explaining! My understanding is that the intention of the no-named-as-default rule is to catch mistakes like

import useState from 'react';

, where the user intends to make a named import (import { useState } from 'react';) and accidentally imports a different value instead.

The import React from 'react'; case feels slightly different to me. When a module contains a “double” default/named export of the same value—something like the following:

export const React = /* ... */
export default React

—the default import is identical to the named import. It’s not useful to warn the user “you might be failing to access the named export you intended,” because the import they are in fact accessing is identical to the named export they’re being warned about.

I think the rule would become much more useful if there were, at minimum, an option on the rule to configure whether this case produces a lint error!

SukkaW added a commit to SukkaW/eslint-plugin-import-x that referenced this issue Aug 29, 2024
SukkaW added a commit to SukkaW/eslint-plugin-import-x that referenced this issue Aug 29, 2024
@SukkaW
Copy link
Collaborator

SukkaW commented Aug 29, 2024

The issue is fixed in #133 and will be included in the next release.

@SukkaW
Copy link
Collaborator

SukkaW commented Aug 29, 2024

eslint-plugin-import-x@4.1.1 has been released!

renovate bot added a commit to mmkal/eslint-plugin-mmkal that referenced this issue Aug 29, 2024
@controversial
Copy link

@SukkaW it looks like eslint-plugin-import just merged a fix for the case I was describing above, which I believe is slightly different than the case that was addressed here

@akwodkiewicz
Copy link

Hey, I'm the author of the PR mentioned above by @controversial. I just got notified about this issue.

You have to be aware that my fix works only for the imports where the objects are re-exported from the file you import.

The API I worked with (probably?) does not give me tools to verify if the named export is equivalent to the default export. In a case of a re-export I'm at least able to check the path of the module and the name of the imported thing. In a case of object definition in the imported file I'm clueless.

So, if a library you're having problems with (lots of false positives) happens to export something as a named and a default export from a barrel file, my fix will work. But otherwise, it won't.

renovate bot added a commit to mmkal/eslint-plugin-mmkal that referenced this issue Sep 17, 2024
##### [v4.2.1](https://github.com/un-ts/eslint-plugin-import-x/blob/HEAD/CHANGELOG.md#421)

##### Patch Changes

-   [#148](un-ts/eslint-plugin-import-x#148) [`d228129`](un-ts/eslint-plugin-import-x@d228129) Thanks [@SukkaW](https://github.com/SukkaW)! - Fix `newline-after-import`'s `considerComments` options when linting `require`, backports import-js/eslint-plugin-import#2952

-   [#147](un-ts/eslint-plugin-import-x#147) [`eca73ed`](un-ts/eslint-plugin-import-x@eca73ed) Thanks [@nchevsky](https://github.com/nchevsky)! - Fix regression in rule `no-unused-modules` which would incorrectly initialize option `src` to `[]` instead of `[process.cwd()]`, breaking file discovery.

-   [#148](un-ts/eslint-plugin-import-x#148) [`d228129`](un-ts/eslint-plugin-import-x@d228129) Thanks [@SukkaW](https://github.com/SukkaW)! - Fix `no-duplicates` for TypeScript, backports import-js/eslint-plugin-import#3033
##### [v4.2.0](https://github.com/un-ts/eslint-plugin-import-x/blob/HEAD/CHANGELOG.md#420)

##### Minor Changes

-   [#142](un-ts/eslint-plugin-import-x#142) [`f12447e`](un-ts/eslint-plugin-import-x@f12447e) Thanks [@Zamiell](https://github.com/Zamiell)! - Add new option "whitelist" for rule "no-extraneous-dependencies"

##### Patch Changes

-   [#146](un-ts/eslint-plugin-import-x#146) [`e5e4580`](un-ts/eslint-plugin-import-x@e5e4580) Thanks [@SukkaW](https://github.com/SukkaW)! - Fix nuxt/eslint#494 by avoid importing from `@typescript-eslint/typescript-estree`.
##### [v4.1.1](https://github.com/un-ts/eslint-plugin-import-x/blob/HEAD/CHANGELOG.md#411)

##### Patch Changes

-   [#133](un-ts/eslint-plugin-import-x#133) [`757ffa9`](un-ts/eslint-plugin-import-x@757ffa9) Thanks [@SukkaW](https://github.com/SukkaW)! - Fix [#123](un-ts/eslint-plugin-import-x#123) where the rule `no-named-as-default` will confuse TypeScript namespace exports with actual exports.
##### [v4.1.0](https://github.com/un-ts/eslint-plugin-import-x/blob/HEAD/CHANGELOG.md#410)

##### Minor Changes

-   [#122](un-ts/eslint-plugin-import-x#122) [`cd52e86`](un-ts/eslint-plugin-import-x@cd52e86) Thanks [@michaelfaith](https://github.com/michaelfaith)! - Add ESLint flat configuration presets. You can access them with:

    ```ts
    import eslintPluginImportX from "eslint-plugin-import-x";

    eslintPluginImportX.flatConfigs.recommended;
    eslintPluginImportX.flatConfigs.react;
    eslintPluginImportX.flatConfigs.typescript;
    eslintPluginImportX.flatConfigs.electron;
    ```

-   [#132](un-ts/eslint-plugin-import-x#132) [`9948c78`](un-ts/eslint-plugin-import-x@9948c78) Thanks [@SukkaW](https://github.com/SukkaW)! - Added `no-rename-default` that forbid importing a default export by a different name. Originally created by [@whitneyit](https://github.com/whitneyit), ported by [@SukkaW](https://github.com/SukkaW)
##### [v4.0.0](https://github.com/un-ts/eslint-plugin-import-x/blob/HEAD/CHANGELOG.md#400)

##### Major Changes

-   [#112](un-ts/eslint-plugin-import-x#112) [`4ba14da`](un-ts/eslint-plugin-import-x@4ba14da) Thanks [@SukkaW](https://github.com/SukkaW)! - Use typescript-eslint v8. The minimum supported ESLint version is now >= 8.57.0 and the minimum required Node.js version is now 18.18.0.
renovate bot added a commit to mmkal/eslint-plugin-mmkal that referenced this issue Sep 17, 2024
##### [v4.2.1](https://github.com/un-ts/eslint-plugin-import-x/blob/HEAD/CHANGELOG.md#421)

##### Patch Changes

-   [#148](un-ts/eslint-plugin-import-x#148) [`d228129`](un-ts/eslint-plugin-import-x@d228129) Thanks [@SukkaW](https://github.com/SukkaW)! - Fix `newline-after-import`'s `considerComments` options when linting `require`, backports import-js/eslint-plugin-import#2952

-   [#147](un-ts/eslint-plugin-import-x#147) [`eca73ed`](un-ts/eslint-plugin-import-x@eca73ed) Thanks [@nchevsky](https://github.com/nchevsky)! - Fix regression in rule `no-unused-modules` which would incorrectly initialize option `src` to `[]` instead of `[process.cwd()]`, breaking file discovery.

-   [#148](un-ts/eslint-plugin-import-x#148) [`d228129`](un-ts/eslint-plugin-import-x@d228129) Thanks [@SukkaW](https://github.com/SukkaW)! - Fix `no-duplicates` for TypeScript, backports import-js/eslint-plugin-import#3033
##### [v4.2.0](https://github.com/un-ts/eslint-plugin-import-x/blob/HEAD/CHANGELOG.md#420)

##### Minor Changes

-   [#142](un-ts/eslint-plugin-import-x#142) [`f12447e`](un-ts/eslint-plugin-import-x@f12447e) Thanks [@Zamiell](https://github.com/Zamiell)! - Add new option "whitelist" for rule "no-extraneous-dependencies"

##### Patch Changes

-   [#146](un-ts/eslint-plugin-import-x#146) [`e5e4580`](un-ts/eslint-plugin-import-x@e5e4580) Thanks [@SukkaW](https://github.com/SukkaW)! - Fix nuxt/eslint#494 by avoid importing from `@typescript-eslint/typescript-estree`.
##### [v4.1.1](https://github.com/un-ts/eslint-plugin-import-x/blob/HEAD/CHANGELOG.md#411)

##### Patch Changes

-   [#133](un-ts/eslint-plugin-import-x#133) [`757ffa9`](un-ts/eslint-plugin-import-x@757ffa9) Thanks [@SukkaW](https://github.com/SukkaW)! - Fix [#123](un-ts/eslint-plugin-import-x#123) where the rule `no-named-as-default` will confuse TypeScript namespace exports with actual exports.
##### [v4.1.0](https://github.com/un-ts/eslint-plugin-import-x/blob/HEAD/CHANGELOG.md#410)

##### Minor Changes

-   [#122](un-ts/eslint-plugin-import-x#122) [`cd52e86`](un-ts/eslint-plugin-import-x@cd52e86) Thanks [@michaelfaith](https://github.com/michaelfaith)! - Add ESLint flat configuration presets. You can access them with:

    ```ts
    import eslintPluginImportX from "eslint-plugin-import-x";

    eslintPluginImportX.flatConfigs.recommended;
    eslintPluginImportX.flatConfigs.react;
    eslintPluginImportX.flatConfigs.typescript;
    eslintPluginImportX.flatConfigs.electron;
    ```

-   [#132](un-ts/eslint-plugin-import-x#132) [`9948c78`](un-ts/eslint-plugin-import-x@9948c78) Thanks [@SukkaW](https://github.com/SukkaW)! - Added `no-rename-default` that forbid importing a default export by a different name. Originally created by [@whitneyit](https://github.com/whitneyit), ported by [@SukkaW](https://github.com/SukkaW)
##### [v4.0.0](https://github.com/un-ts/eslint-plugin-import-x/blob/HEAD/CHANGELOG.md#400)

##### Major Changes

-   [#112](un-ts/eslint-plugin-import-x#112) [`4ba14da`](un-ts/eslint-plugin-import-x@4ba14da) Thanks [@SukkaW](https://github.com/SukkaW)! - Use typescript-eslint v8. The minimum supported ESLint version is now >= 8.57.0 and the minimum required Node.js version is now 18.18.0.
renovate bot added a commit to mmkal/eslint-plugin-mmkal that referenced this issue Sep 17, 2024
##### [v4.2.1](https://github.com/un-ts/eslint-plugin-import-x/blob/HEAD/CHANGELOG.md#421)

##### Patch Changes

-   [#148](un-ts/eslint-plugin-import-x#148) [`d228129`](un-ts/eslint-plugin-import-x@d228129) Thanks [@SukkaW](https://github.com/SukkaW)! - Fix `newline-after-import`'s `considerComments` options when linting `require`, backports import-js/eslint-plugin-import#2952

-   [#147](un-ts/eslint-plugin-import-x#147) [`eca73ed`](un-ts/eslint-plugin-import-x@eca73ed) Thanks [@nchevsky](https://github.com/nchevsky)! - Fix regression in rule `no-unused-modules` which would incorrectly initialize option `src` to `[]` instead of `[process.cwd()]`, breaking file discovery.

-   [#148](un-ts/eslint-plugin-import-x#148) [`d228129`](un-ts/eslint-plugin-import-x@d228129) Thanks [@SukkaW](https://github.com/SukkaW)! - Fix `no-duplicates` for TypeScript, backports import-js/eslint-plugin-import#3033
##### [v4.2.0](https://github.com/un-ts/eslint-plugin-import-x/blob/HEAD/CHANGELOG.md#420)

##### Minor Changes

-   [#142](un-ts/eslint-plugin-import-x#142) [`f12447e`](un-ts/eslint-plugin-import-x@f12447e) Thanks [@Zamiell](https://github.com/Zamiell)! - Add new option "whitelist" for rule "no-extraneous-dependencies"

##### Patch Changes

-   [#146](un-ts/eslint-plugin-import-x#146) [`e5e4580`](un-ts/eslint-plugin-import-x@e5e4580) Thanks [@SukkaW](https://github.com/SukkaW)! - Fix nuxt/eslint#494 by avoid importing from `@typescript-eslint/typescript-estree`.
##### [v4.1.1](https://github.com/un-ts/eslint-plugin-import-x/blob/HEAD/CHANGELOG.md#411)

##### Patch Changes

-   [#133](un-ts/eslint-plugin-import-x#133) [`757ffa9`](un-ts/eslint-plugin-import-x@757ffa9) Thanks [@SukkaW](https://github.com/SukkaW)! - Fix [#123](un-ts/eslint-plugin-import-x#123) where the rule `no-named-as-default` will confuse TypeScript namespace exports with actual exports.
##### [v4.1.0](https://github.com/un-ts/eslint-plugin-import-x/blob/HEAD/CHANGELOG.md#410)

##### Minor Changes

-   [#122](un-ts/eslint-plugin-import-x#122) [`cd52e86`](un-ts/eslint-plugin-import-x@cd52e86) Thanks [@michaelfaith](https://github.com/michaelfaith)! - Add ESLint flat configuration presets. You can access them with:

    ```ts
    import eslintPluginImportX from "eslint-plugin-import-x";

    eslintPluginImportX.flatConfigs.recommended;
    eslintPluginImportX.flatConfigs.react;
    eslintPluginImportX.flatConfigs.typescript;
    eslintPluginImportX.flatConfigs.electron;
    ```

-   [#132](un-ts/eslint-plugin-import-x#132) [`9948c78`](un-ts/eslint-plugin-import-x@9948c78) Thanks [@SukkaW](https://github.com/SukkaW)! - Added `no-rename-default` that forbid importing a default export by a different name. Originally created by [@whitneyit](https://github.com/whitneyit), ported by [@SukkaW](https://github.com/SukkaW)
##### [v4.0.0](https://github.com/un-ts/eslint-plugin-import-x/blob/HEAD/CHANGELOG.md#400)

##### Major Changes

-   [#112](un-ts/eslint-plugin-import-x#112) [`4ba14da`](un-ts/eslint-plugin-import-x@4ba14da) Thanks [@SukkaW](https://github.com/SukkaW)! - Use typescript-eslint v8. The minimum supported ESLint version is now >= 8.57.0 and the minimum required Node.js version is now 18.18.0.
renovate bot added a commit to mmkal/eslint-plugin-mmkal that referenced this issue Sep 17, 2024
##### [v4.2.1](https://github.com/un-ts/eslint-plugin-import-x/blob/HEAD/CHANGELOG.md#421)

##### Patch Changes

-   [#148](un-ts/eslint-plugin-import-x#148) [`d228129`](un-ts/eslint-plugin-import-x@d228129) Thanks [@SukkaW](https://github.com/SukkaW)! - Fix `newline-after-import`'s `considerComments` options when linting `require`, backports import-js/eslint-plugin-import#2952

-   [#147](un-ts/eslint-plugin-import-x#147) [`eca73ed`](un-ts/eslint-plugin-import-x@eca73ed) Thanks [@nchevsky](https://github.com/nchevsky)! - Fix regression in rule `no-unused-modules` which would incorrectly initialize option `src` to `[]` instead of `[process.cwd()]`, breaking file discovery.

-   [#148](un-ts/eslint-plugin-import-x#148) [`d228129`](un-ts/eslint-plugin-import-x@d228129) Thanks [@SukkaW](https://github.com/SukkaW)! - Fix `no-duplicates` for TypeScript, backports import-js/eslint-plugin-import#3033
##### [v4.2.0](https://github.com/un-ts/eslint-plugin-import-x/blob/HEAD/CHANGELOG.md#420)

##### Minor Changes

-   [#142](un-ts/eslint-plugin-import-x#142) [`f12447e`](un-ts/eslint-plugin-import-x@f12447e) Thanks [@Zamiell](https://github.com/Zamiell)! - Add new option "whitelist" for rule "no-extraneous-dependencies"

##### Patch Changes

-   [#146](un-ts/eslint-plugin-import-x#146) [`e5e4580`](un-ts/eslint-plugin-import-x@e5e4580) Thanks [@SukkaW](https://github.com/SukkaW)! - Fix nuxt/eslint#494 by avoid importing from `@typescript-eslint/typescript-estree`.
##### [v4.1.1](https://github.com/un-ts/eslint-plugin-import-x/blob/HEAD/CHANGELOG.md#411)

##### Patch Changes

-   [#133](un-ts/eslint-plugin-import-x#133) [`757ffa9`](un-ts/eslint-plugin-import-x@757ffa9) Thanks [@SukkaW](https://github.com/SukkaW)! - Fix [#123](un-ts/eslint-plugin-import-x#123) where the rule `no-named-as-default` will confuse TypeScript namespace exports with actual exports.
##### [v4.1.0](https://github.com/un-ts/eslint-plugin-import-x/blob/HEAD/CHANGELOG.md#410)

##### Minor Changes

-   [#122](un-ts/eslint-plugin-import-x#122) [`cd52e86`](un-ts/eslint-plugin-import-x@cd52e86) Thanks [@michaelfaith](https://github.com/michaelfaith)! - Add ESLint flat configuration presets. You can access them with:

    ```ts
    import eslintPluginImportX from "eslint-plugin-import-x";

    eslintPluginImportX.flatConfigs.recommended;
    eslintPluginImportX.flatConfigs.react;
    eslintPluginImportX.flatConfigs.typescript;
    eslintPluginImportX.flatConfigs.electron;
    ```

-   [#132](un-ts/eslint-plugin-import-x#132) [`9948c78`](un-ts/eslint-plugin-import-x@9948c78) Thanks [@SukkaW](https://github.com/SukkaW)! - Added `no-rename-default` that forbid importing a default export by a different name. Originally created by [@whitneyit](https://github.com/whitneyit), ported by [@SukkaW](https://github.com/SukkaW)
##### [v4.0.0](https://github.com/un-ts/eslint-plugin-import-x/blob/HEAD/CHANGELOG.md#400)

##### Major Changes

-   [#112](un-ts/eslint-plugin-import-x#112) [`4ba14da`](un-ts/eslint-plugin-import-x@4ba14da) Thanks [@SukkaW](https://github.com/SukkaW)! - Use typescript-eslint v8. The minimum supported ESLint version is now >= 8.57.0 and the minimum required Node.js version is now 18.18.0.
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