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

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

whitneyit
Copy link

Closes: #1041

@whitneyit whitneyit force-pushed the main branch 2 times, most recently from d46657f to ee3fd7e Compare May 3, 2024 04:58
@whitneyit
Copy link
Author

Added more tests

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

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,

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
Copy link
Member

ljharb commented May 3, 2024

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
Copy link
Author

whitneyit commented May 3, 2024

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
Copy link
Member

ljharb commented May 3, 2024

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
Copy link
Author

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
Copy link
Member

ljharb commented May 3, 2024

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
Copy link
Author

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
Copy link
Author

whitneyit commented May 3, 2024

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 whitneyit force-pushed the main branch 4 times, most recently from 51d4f6d to ea0fc45 Compare May 3, 2024 08:04
@whitneyit
Copy link
Author

whitneyit commented May 3, 2024

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 whitneyit force-pushed the main branch 2 times, most recently from 1e4d2c0 to 6788026 Compare May 3, 2024 14:40
@whitneyit
Copy link
Author

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

Copy link

codecov bot commented May 8, 2024

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
Copy link
Author

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.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased and cleaned up a few things.

type: 'boolean',
},
preventRenamingBindings: {
default: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

booleans should default to false; can we rename this?

@ljharb ljharb marked this pull request as draft August 30, 2024 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Rule proposal: no-rename-default
2 participants