Added more tests

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,

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 eitherfoo orbar. I personally would usefunc-name-matching to side step this problem entirely. I would settle forfoo because of the lineexport 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.

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 aboutexport { foo as default } from 'blah'? would that need to be named "foo" in the importer of this module?

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

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.jsexport{defaultasButton}from'./Button';export{defaultasCard}from'./Card';export{defaultasDatePicker}from'./DatePicker';

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

// src/components/Button/Button.jsexportfunctionButton(){};// src/components/Button/index.jsexport{Buttonasdefault}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?

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 totrue, 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 tofalse). Thorough documentation, including examples, will help here.

It would benice 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.

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:

typeOptions={commonjs?:boolean;}

The proposed future options may be:

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

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.

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

typeOptions={commonjs?:boolean;// default falsepreventRenamingDeclarations?:boolean;// default true - Bullet point 1preventRenamingBindings?:boolean;// default true - Bullet point 2}

The usage would be:

// preventRenamingDeclarations prevents renaming thisexportdefaultasyncfunctiongetUsers(){}

asyncfunctiongetUsers(){}// preventRenamingBindings prevents renaming thisexportdefaultgetUsers;

I though of another use case:

// middleware/logger.jsexportdefaultfunctionwithLogger(fn){returnfunctioninnerLogger(...args){console.log(`${fn.name} called`);returnfn.apply(null,args);}}

// api/get-users.jsimportwithLoggerfrom'../middleware/logger';asyncfunctiongetUsers(){}exportdefaultwithLogger(getUsers)

Would this begetUsers,withLogger, orinnerLogger?

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

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 thisexportdefaultasyncfunctiongetUsers(){}

asyncfunctiongetUsers(){}// Yet prevent renaming this?exportdefaultgetUsers;

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 attests/src/rules/no-rename-default.js

For this example:

// binding-fn-rename.jsconstfoo=functionbar(){};exportdefaultfoo;// valid.jsimportfoofrom'./binding-fn-rename.js';// invalid.jsimportbarfrom'./binding-fn-rename.js';

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

@ljharb I have implemented thepreventRenamingBindings option. It basically disabled checking the rule againstIdentifier andAssignmentExpressions.

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

Codecov Report

Attention: Patch coverage is85.71429% with18 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.

@@            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.

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.