[New] no-rename-default: Forbid importing a default export by a different name

[whitneyit]

Closes: #1041

[whitneyit]

Added more tests

[ljharb]

What happens if the default-exported item doesn't have a name?

What if it's const foo = function bar() {}; export default foo;?

What if the filename is a reserved word, like default or class?

What if a file is structured like ./foo/bar.js, can it be imported as fooBar? bar?

What if a file is structured like ./foo/index.js, can it be imported as foo? index?

[whitneyit]

Hi @ljharb, the conversation regarding #1041 has been muddied regarding filenames.

This PR does not concern itself with filenames. It is strictly focused on renaming the default export. Nothing more. Nothing less.

To answer your questions:

What happens if the default-exported item doesn't have a name?

This would have no bearing on this PR. I have tested for this with the following,

eslint-plugin-import/tests/src/rules/no-rename-default.js

Line 11 in ee3fd7e

import _ from './no-rename-default/anon.js'

What if it's const foo = function bar() {}; export default foo;?

This would be an interesting case that I have not considered. I could see an argument being made for either foo or bar. I personally would use func-name-matching to side step this problem entirely. I would settle for foo because of the line export default foo.

What if the filename is a reserved word, like default or class?

N/A.

What if a file is structured like ./foo/bar.js, can it be imported as fooBar? bar?

N/A.

What if a file is structured like ./foo/index.js, can it be imported as foo? index?

N/A.

[ljharb]

ok, so then, what this rule is currently doing is:

• for any default export that inherently has a name (ie, a named class or named non-arrow function), the import binding must match
• for any default export that is created separately from the export statement, named or not, the name of the binding that's default-exported will be the required import binding name
  ?

what about export { foo as default } from 'blah'? would that need to be named "foo" in the importer of this module?

what about export { default as foo } from 'blah' where the required "blah" default import name is "bar"? would that error because it's not being re-exported as bar? or would that skip this rule entirely?

[whitneyit]

I thought about those export cases and was curious to hear feedback from others.

A common case that I have seen is to re-export a component library as a "barrel file" of sorts.

For example:

// src/components/index.js
export { default as Button } from './Button';
export { default as Card } from './Card';
export { default as DatePicker } from './DatePicker';

The reverse is also not too uncommon when wrapping a "folder style" component into a single export:

// src/components/Button/Button.js
export function Button() {};

// src/components/Button/index.js
export { Button as default } from './Button';

Both of these cases above would be skipped with the current approach of the rule. Perhaps the rule could be renamed to show the renaming on the import side, (I'm not too fond of this approach - the new name will probably sound quite clunky).

Alternatively, we could add additional documentation to show the renaming only causing issues during import. What do you think @ljharb?

[ljharb]

Barrel files are indeed common, but they're a horrific practice that makes programs and bundles much larger and slower.

I think that it would probably be best to configure this as two separate options (one for each bullet point above), that default to true, and then we can add additional options if it turns out there's a need to handle re-exports and other edge cases (that would default to false). Thorough documentation, including examples, will help here.

It would be nice to account for those in the initial release of the rule, but if we aren't confident about how they should work, it's better to add the options later.

[whitneyit]

I'm not sure if we would need options for the two bullet points that you mentioned:

• for any default export that inherently has a name (ie, a named class or named non-arrow function), the import binding must match
• for any default export that is created separately from the export statement, named or not, the name of the binding that's default-exported will be the required import binding name

The rule itself is the option.

If you want to prevent those two cases above, enable the rule, (that's the option the user is buying into), if you don't want it, don't enable the rule.

Is that what you mean @ljharb, perhaps I am missing something ...

I agree about adding options for "barrel" exports down the line, if there is enough interest.

The options that are currently support are:

type Options = {
  commonjs?: boolean;
}

The proposed future options may be:

type Options = {
  commonjs?: boolean;
  disallowRenamingDefaultsOnExport?: boolean; // Not the best name ...
}

[ljharb]

I’m assuming that folks may only want one of those two bullet points (for example, I’d only want the first of those two), and in a future where there’s 3 or 4 or 5 options, we’ll want them to be independently configurable. Anticipating that, it seems useful to start with that schema in mind.

[whitneyit]

Ok, so that we're on the same page. Is this what you're thinking?

type Options = {
  commonjs?: boolean; // default false
  preventRenamingDeclarations?: boolean; // default true - Bullet point 1
  preventRenamingBindings?: boolean; // default true - Bullet point 2
}

The usage would be:

// preventRenamingDeclarations prevents renaming this
export default async function getUsers() {}

async function getUsers() {}

// preventRenamingBindings prevents renaming this
export default getUsers;

[whitneyit]

I though of another use case:

// middleware/logger.js
export default function withLogger(fn) {
  return function innerLogger(...args) {
    console.log(`${fn.name} called`);
    return fn.apply(null, args);
  }
}

// api/get-users.js
import withLogger from '../middleware/logger';

async function getUsers() {}

export default withLogger(getUsers)

Would this be getUsers, withLogger, or innerLogger?

Update: I now recursively unwrap the argument nodes, and in this case, would return 'getUsers'.

[whitneyit]

Thinking about this more @ljharb, in relation to your two bullet points:

• for any default export that inherently has a name (ie, a named class or named non-arrow function), the import binding must match
• for any default export that is created separately from the export statement, named or not, the name of the binding that's default-exported will be the required import binding name

I can not think of a situation where a user would want to allow renaming the first bullet, yet disable renaming the second bullet:

// Who would want to allow renaming this
export default async function getUsers() {}

async function getUsers() {}

// Yet prevent renaming this?
export default getUsers;

I think for the sake of the api surface, we only need to include an option for the second bullet point. I still feel that the rule itself will double as an option for the first bullet point.

Regardless of my opinion, I'm open to taking your direction on this. I'll begin work on enable that second option. In the mean time.

Also, I have added tests for all of the questions that you have raised thus far. Have a look at tests/src/rules/no-rename-default.js

For this example:

// binding-fn-rename.js
const foo = function bar() {};
export default foo;

// valid.js
import foo from './binding-fn-rename.js';

// invalid.js
import bar from './binding-fn-rename.js';

The direction that I went with was to mark foo as the correct decision.

[whitneyit]

@ljharb I have implemented the preventRenamingBindings option. It basically disabled checking the rule against Identifier and AssignmentExpressions.

I added more tests for classes, generators, and a few others.

[codecov]

Codecov Report

Attention: Patch coverage is 85.71429% with 18 lines in your changes missing coverage. Please review.

Project coverage is 95.32%. Comparing base (c0ac54b) to head (b8c6a51).
Report is 4 commits behind head on main.

Files	Patch %	Lines
src/rules/no-rename-default.js	83.17%	18 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3006      +/-   ##
==========================================
- Coverage   95.66%   95.32%   -0.35%     
==========================================
  Files          78       79       +1     
  Lines        3275     3398     +123     
  Branches     1150     1195      +45     
==========================================
+ Hits         3133     3239     +106     
- Misses        142      159      +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[whitneyit]

Hi @ljharb, is there any other tests that you think that I should add for this?

I will start working on the documentation in the meantime.

[ljharb]

Rebased and cleaned up a few things.

[ljharb]

booleans should default to false; can we rename this?