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 warn/error when value is both named export and default export #1594

Closed
t-ricci-enhancers opened this issue Jan 5, 2020 · 10 comments · Fixed by #3032
Closed

Comments

@t-ricci-enhancers
Copy link

t-ricci-enhancers commented Jan 5, 2020

Example (here I want to export everything from one or merge more files, then set a default export):

// filename is: ./file/index.js
export * from './file'; // this contains export const file = ...
export * from './anotherfile';
export {file as default} from './file';

// filename is ./main.js
import file from './file';  // import from directory file, resolving to ./file/index.js
/*
 Line above gives warn/error:
 Using exported name 'file' as identifier
 for default export.eslint(import/no-named-as-default)
*/
@ljharb
Copy link
Member

ljharb commented Jan 5, 2020

This is intentional; since there’s a named export named “file”, the rule won’t let you use that name for the default when importing.

@markmsmith
Copy link

What about a simpler example:

// Bar.js

/** @private visible for testing */
export const foo = 42;

/** @private visible for testing */
export class BarHelper{}

export class Bar {
  
}

export default Bar;
// Bar.test.js

import {foo, Bar, BarHelper} from '../src/Bar.js'
// write tests against Bar
// index.js

// no-named-as-default error here
import Bar from './Bar.js';
// use Bar for real
const bar = new Bar();

In this case it will complain about the Bar import in index.js, even though it's referring to the exact same thing. We've used this pattern in a number of projects to indicate that the default export is the public contract for the main thing you're supposed to use, but individual named exports would be used by unit tests.

@t-ricci-enhancers
Copy link
Author

@markmsmith yeah, your example is more clear. Still I find that the issue triggering the warn/error of eslint is annoying for these use cases

@ljharb
Copy link
Member

ljharb commented Jan 10, 2020

That makes no sense to me why you’d double-export Bar. Tests should be testing the same thing production uses - which in your case is the default export.

@t-ricci-enhancers
Copy link
Author

Yeah, but I'm making a npm package and each "submodule" folder as an index file that re-exports everything + a default. It was easier to do it and have most of the "submodule" defaults only accessible by name (not default) outside of the npm package

@Nokel81
Copy link
Contributor

Nokel81 commented Aug 12, 2021

I have run into this too when trying to consume some npm packages. For instance the https://www.npmjs.com/package/abort-controller has the following exports:

export default AbortController
export { AbortController, AbortSignal }

It would be nice if doing import AbortController from "abort-controller"; wouldn't warn because it is the same item.

I prefer keeping the rule in place so that if a package does the following:

export default foo;
export { bar, bat };

I do get a warning if I do import bar from "some-package"

@ljharb
Copy link
Member

ljharb commented Aug 12, 2021

While I continue to think that it's utter nonsense to export the same value twice, I agree that in that case only, the rule should not warn.

I'd be happy to accept a PR that handled that (perhaps under an option)

@Nokel81
Copy link
Contributor

Nokel81 commented Aug 12, 2021

Fair enough, it does seem like a really bad practice and I think it should be under an option.

@akwodkiewicz
Copy link
Contributor

I can try this. I have lots of "false positives" for

import userEvent from '@testing-library/user-event';

I'd like to get rid of. Forking the repo right now.

@akwodkiewicz
Copy link
Contributor

akwodkiewicz commented Sep 8, 2024

I'm back at this and I'm trying to cover all possible cases. It's my first contribution to eslint-plugin-import (or ESLint in general), so I'm not very fluent in all the available AST APIs and the data available at the linting stage.

But what I have found so far it's that there are 2 distinct cases of this problem:

  1. The exported value1 is re-exported from some other file:

    export { something as default } from './file';
    export { something } from './file';

    Example:
    https://github.com/testing-library/user-event/blob/4019ceeabad2538da3f69c63bab79be731b99f3f/src/index.ts#L1-L4

  2. The exported value is defined in the file that exports it:

    const something = 1;
    export { something as default };
    export { something };

    No known example in known libs.

I am able to address the case no. 1, because the ExportMap structure contains metadata that eventually allows me to verify that both re-exports point to something from ./file.

But I don't thing I'm able to verify that in the case no. 2 something and default are actually pointing to the same something = 1 value.

Or maybe I reached a dead-end trying to solve the case no. 2 with the ExportMap (ExportMapBuilder, importDeclaration.js util). Honestly, I am not fully aware of how they work precisely, I'm just tinkering with this stuff.

If you have some ideas on which API I should use to solve the case no. 2 please let me know. For now I'll try to clean-up the PR with the solution for the re-export case.

Footnotes

  1. I mean here a simple variable, function, object, etc.

akwodkiewicz added a commit to akwodkiewicz/eslint-plugin-import that referenced this issue Sep 8, 2024
akwodkiewicz added a commit to akwodkiewicz/eslint-plugin-import that referenced this issue Sep 8, 2024
ljharb pushed a commit to akwodkiewicz/eslint-plugin-import that referenced this issue Sep 9, 2024
akwodkiewicz added a commit to akwodkiewicz/eslint-plugin-import that referenced this issue Sep 9, 2024
ljharb pushed a commit to akwodkiewicz/eslint-plugin-import that referenced this issue Sep 9, 2024
…is both a named and a default export

 - add tests for import-js#1594
sparten11740 pushed a commit to ExodusMovement/eslint-plugin-import that referenced this issue Nov 21, 2024
…is both a named and a default export

 - add tests for import-js#1594
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants