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

[Fix] no-named-as-default: allow named specifier if equivalent to default #3032

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange

### Fixed
- `ExportMap` / flat config: include `languageOptions` in context ([#3052], thanks [@michaelfaith])
- [`no-named-as-default`]: Allow using an identifier if the export is both a named and a default export ([#3032], thanks [@akwodkiewicz])

## [2.30.0] - 2024-09-02

Expand Down Expand Up @@ -1139,6 +1140,7 @@ for info on changes for earlier releases.
[#3043]: https://github.com/import-js/eslint-plugin-import/pull/3043
[#3036]: https://github.com/import-js/eslint-plugin-import/pull/3036
[#3033]: https://github.com/import-js/eslint-plugin-import/pull/3033
[#3032]: https://github.com/import-js/eslint-plugin-import/pull/3032
[#3018]: https://github.com/import-js/eslint-plugin-import/pull/3018
[#3012]: https://github.com/import-js/eslint-plugin-import/pull/3012
[#3011]: https://github.com/import-js/eslint-plugin-import/pull/3011
Expand Down Expand Up @@ -1726,6 +1728,7 @@ for info on changes for earlier releases.
[@AdriAt360]: https://github.com/AdriAt360
[@ai]: https://github.com/ai
[@aks-]: https://github.com/aks-
[@akwodkiewicz]: https://github.com/akwodkiewicz
[@aladdin-add]: https://github.com/aladdin-add
[@alex-page]: https://github.com/alex-page
[@alexgorbatchev]: https://github.com/alexgorbatchev
Expand Down
63 changes: 53 additions & 10 deletions src/rules/no-named-as-default.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,28 +15,71 @@

create(context) {
function checkDefault(nameKey, defaultSpecifier) {
/**
* For ImportDefaultSpecifier we're interested in the "local" name (`foo` for `import {bar as foo} ...`)
* For ExportDefaultSpecifier we're interested in the "exported" name (`foo` for `export {bar as foo} ...`)
*/
const analyzedName = defaultSpecifier[nameKey].name;

Check warning on line 22 in src/rules/no-named-as-default.js

View check run for this annotation

Codecov / codecov/patch

src/rules/no-named-as-default.js#L22

Added line #L22 was not covered by tests

// #566: default is a valid specifier
if (defaultSpecifier[nameKey].name === 'default') { return; }
if (analyzedName === 'default') { return; }

Check warning on line 25 in src/rules/no-named-as-default.js

View check run for this annotation

Codecov / codecov/patch

src/rules/no-named-as-default.js#L25

Added line #L25 was not covered by tests

const declaration = importDeclaration(context, defaultSpecifier);
/** @type {import('../exportMap').default | null} */
const importedModule = ExportMapBuilder.get(declaration.source.value, context);
if (importedModule == null) { return; }

Check warning on line 30 in src/rules/no-named-as-default.js

View check run for this annotation

Codecov / codecov/patch

src/rules/no-named-as-default.js#L29-L30

Added lines #L29 - L30 were not covered by tests

const imports = ExportMapBuilder.get(declaration.source.value, context);
if (imports == null) { return; }
if (importedModule.errors.length > 0) {
importedModule.reportErrors(context, declaration);
return;

Check warning on line 34 in src/rules/no-named-as-default.js

View check run for this annotation

Codecov / codecov/patch

src/rules/no-named-as-default.js#L32-L34

Added lines #L32 - L34 were not covered by tests
}

if (imports.errors.length) {
imports.reportErrors(context, declaration);
if (!importedModule.hasDefault) {

Check warning on line 37 in src/rules/no-named-as-default.js

View check run for this annotation

Codecov / codecov/patch

src/rules/no-named-as-default.js#L37

Added line #L37 was not covered by tests
// The rule is triggered for default imports/exports, so if the imported module has no default
// this means we're dealing with incorrect source code anyway
return;
}

if (imports.has('default') && imports.has(defaultSpecifier[nameKey].name)) {
if (!importedModule.has(analyzedName)) {

Check warning on line 43 in src/rules/no-named-as-default.js

View check run for this annotation

Codecov / codecov/patch

src/rules/no-named-as-default.js#L43

Added line #L43 was not covered by tests
// The name used locally for the default import was not even used in the imported module.
return;

Check warning on line 45 in src/rules/no-named-as-default.js

View check run for this annotation

Codecov / codecov/patch

src/rules/no-named-as-default.js#L45

Added line #L45 was not covered by tests
}

context.report(
defaultSpecifier,
`Using exported name '${defaultSpecifier[nameKey].name}' as identifier for default export.`,
);
/**
* FIXME: We can verify if a default and a named export are pointing to the same symbol only
* if they are both `reexports`. In case one of the symbols is not a re-export, but defined
* in the file, the ExportMap structure has no info about what actually is being exported --
* the value in the `namespace` Map is an empty object.
*
* To solve this, it would require not relying on the ExportMap, but on some other way of
* accessing the imported module and its exported values.
*
* Additionally, although `ExportMap.get` is a unified way to get info from both `reexports`
* and `namespace` maps, it does not return valid output we need here, and I think this is
* related to the "cycle safeguards" in the `get` function.
*/

if (importedModule.reexports.has(analyzedName) && importedModule.reexports.has('default')) {
const thingImportedWithNamedImport = importedModule.reexports.get(analyzedName).getImport();
const thingImportedWithDefaultImport = importedModule.reexports.get('default').getImport();

Check warning on line 64 in src/rules/no-named-as-default.js

View check run for this annotation

Codecov / codecov/patch

src/rules/no-named-as-default.js#L62-L64

Added lines #L62 - L64 were not covered by tests

// Case: both imports point to the same file and they both refer to the same symbol in this file.
if (
thingImportedWithNamedImport.path === thingImportedWithDefaultImport.path
&& thingImportedWithNamedImport.local === thingImportedWithDefaultImport.local

Check warning on line 69 in src/rules/no-named-as-default.js

View check run for this annotation

Codecov / codecov/patch

src/rules/no-named-as-default.js#L67-L69

Added lines #L67 - L69 were not covered by tests
) {
// #1594: the imported module exports the same thing via a default export and a named export
return;

Check warning on line 72 in src/rules/no-named-as-default.js

View check run for this annotation

Codecov / codecov/patch

src/rules/no-named-as-default.js#L72

Added line #L72 was not covered by tests
}
}

context.report(

Check warning on line 76 in src/rules/no-named-as-default.js

View check run for this annotation

Codecov / codecov/patch

src/rules/no-named-as-default.js#L76

Added line #L76 was not covered by tests
defaultSpecifier,
`Using exported name '${defaultSpecifier[nameKey].name}' as identifier for default ${nameKey === 'local' ? `import` : `export`}.`,

Check warning on line 78 in src/rules/no-named-as-default.js

View check run for this annotation

Codecov / codecov/patch

src/rules/no-named-as-default.js#L78

Added line #L78 was not covered by tests
);

}

return {
ImportDefaultSpecifier: checkDefault.bind(null, 'local'),
ExportDefaultSpecifier: checkDefault.bind(null, 'exported'),
Expand Down
4 changes: 4 additions & 0 deletions tests/files/no-named-as-default/exports.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
const variable = 1;

export { variable };
export default variable;
2 changes: 2 additions & 0 deletions tests/files/no-named-as-default/misleading-re-exports.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export { variable as something } from './exports';
export { something as default } from './something';
1 change: 1 addition & 0 deletions tests/files/no-named-as-default/no-default-export.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const foobar = 4;
2 changes: 2 additions & 0 deletions tests/files/no-named-as-default/re-exports.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export { something as default } from "./something";
export { something } from "./something";
1 change: 1 addition & 0 deletions tests/files/no-named-as-default/something.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const something = 42;
110 changes: 96 additions & 14 deletions tests/src/rules/no-named-as-default.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,49 +12,105 @@ ruleTester.run('no-named-as-default', rule, {
test({ code: 'import bar, { foo } from "./empty-folder";' }),

// es7
test({ code: 'export bar, { foo } from "./bar";',
parser: parsers.BABEL_OLD }),
test({ code: 'export bar from "./bar";',
parser: parsers.BABEL_OLD }),
test({
code: 'export bar, { foo } from "./bar";',
parser: parsers.BABEL_OLD,
}),
test({
code: 'export bar from "./bar";',
parser: parsers.BABEL_OLD,
}),

// #566: don't false-positive on `default` itself
test({ code: 'export default from "./bar";',
parser: parsers.BABEL_OLD }),
test({
code: 'export default from "./bar";',
parser: parsers.BABEL_OLD,
}),

// es2022: Arbitrary module namespae identifier names
testVersion('>= 8.7', () => ({
code: 'import bar, { foo } from "./export-default-string-and-named"',
parserOptions: { ecmaVersion: 2022 },
})),

// #1594: Allow importing as default if object is exported both as default and named
test({ code: 'import something from "./no-named-as-default/re-exports.js";' }),
test({
code: 'import { something } from "./no-named-as-default/re-exports.js";',
}),
test({
code: 'import myOwnNameForVariable from "./no-named-as-default/exports.js";',
}),
test({
code: 'import { variable } from "./no-named-as-default/exports.js";',
}),
test({
code: 'import variable from "./no-named-as-default/misleading-re-exports.js";',
}),
test({
// incorrect import
code: 'import foobar from "./no-named-as-default/no-default-export.js";',
}),
// same tests, but for exports
test({
code: 'export something from "./no-named-as-default/re-exports.js";',
parser: parsers.BABEL_OLD,
}),
test({
code: 'export { something } from "./no-named-as-default/re-exports.js";',
}),
test({
code: 'export myOwnNameForVariable from "./no-named-as-default/exports.js";',
parser: parsers.BABEL_OLD,
}),
test({
code: 'export { variable } from "./no-named-as-default/exports.js";',
}),
test({
code: 'export variable from "./no-named-as-default/misleading-re-exports.js";',
parser: parsers.BABEL_OLD,
}),
test({
code: 'export foobar from "./no-named-as-default/no-default-export.js";',
parser: parsers.BABEL_OLD,
}),

...SYNTAX_CASES,
),

invalid: [].concat(
test({
code: 'import foo from "./bar";',
errors: [{
message: 'Using exported name \'foo\' as identifier for default export.',
type: 'ImportDefaultSpecifier' }] }),
message: 'Using exported name \'foo\' as identifier for default import.',
type: 'ImportDefaultSpecifier',
}],
}),
test({
code: 'import foo, { foo as bar } from "./bar";',
errors: [{
message: 'Using exported name \'foo\' as identifier for default export.',
type: 'ImportDefaultSpecifier' }] }),
message: 'Using exported name \'foo\' as identifier for default import.',
type: 'ImportDefaultSpecifier',
}],
}),

// es7
test({
code: 'export foo from "./bar";',
parser: parsers.BABEL_OLD,
errors: [{
message: 'Using exported name \'foo\' as identifier for default export.',
type: 'ExportDefaultSpecifier' }] }),
type: 'ExportDefaultSpecifier',
}],
}),
test({
code: 'export foo, { foo as bar } from "./bar";',
parser: parsers.BABEL_OLD,
errors: [{
message: 'Using exported name \'foo\' as identifier for default export.',
type: 'ExportDefaultSpecifier' }] }),
type: 'ExportDefaultSpecifier',
}],
}),

test({
code: 'import foo from "./malformed.js"',
Expand All @@ -68,18 +124,44 @@ ruleTester.run('no-named-as-default', rule, {
testVersion('>= 8.7', () => ({
code: 'import foo from "./export-default-string-and-named"',
errors: [{
message: 'Using exported name \'foo\' as identifier for default export.',
message: 'Using exported name \'foo\' as identifier for default import.',
type: 'ImportDefaultSpecifier',
}],
parserOptions: { ecmaVersion: 2022 },
})),
testVersion('>= 8.7', () => ({
code: 'import foo, { foo as bar } from "./export-default-string-and-named"',
errors: [{
message: 'Using exported name \'foo\' as identifier for default export.',
message: 'Using exported name \'foo\' as identifier for default import.',
type: 'ImportDefaultSpecifier',
}],
parserOptions: { ecmaVersion: 2022 },
})),

// #1594: Allow importing as default if object is exported both as default and named
test({
code: 'import something from "./no-named-as-default/misleading-re-exports.js";',
parser: parsers.BABEL_OLD,
errors: [{
message: 'Using exported name \'something\' as identifier for default import.',
type: 'ImportDefaultSpecifier',
}],
}),
// The only cases that are not covered by the fix
test({
code: 'import variable from "./no-named-as-default/exports.js";',
errors: [{
message: 'Using exported name \'variable\' as identifier for default import.',
type: 'ImportDefaultSpecifier',
}],
}),
test({
code: 'export variable from "./no-named-as-default/exports.js";',
parser: parsers.BABEL_OLD,
errors: [{
message: 'Using exported name \'variable\' as identifier for default export.',
type: 'ExportDefaultSpecifier',
}],
}),
),
});