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

Rule proposal: no-rename-default #1041

Open
steve-taylor opened this issue Mar 12, 2018 · 26 comments · May be fixed by #1143 or #3006
Open

Rule proposal: no-rename-default #1041

steve-taylor opened this issue Mar 12, 2018 · 26 comments · May be fixed by #1143 or #3006

Comments

@steve-taylor
Copy link

Consider the following:

components/foo.js:

export default class Foo extends React.Component {
    render() {
        // ...
    }
}

widgets/foo.js:

import FooComponent from '../components/foo';

export default class Foo extends BaseWidget {
    constructor() {
        super(FooComponent);
    }

    // ...
}

Foo (in components/foo.js) and FooComponent are exactly the same thing, but have been renamed via default import/export. When one thing has two or more names, it increases the difficulty of comprehending code, especially when there are many functions, classes, etc. that are renamed on import.

The proposal is to have a rule that generates a warning or error when a default import is given a different name than a default export.

@ljharb
Copy link
Member

ljharb commented Mar 12, 2018

This is a valuable part of both default and named import syntax (the ability to rename things), and I’m not sure why it’s an issue that needs a rule.

@steve-taylor
Copy link
Author

It’s an issue for the reasons outlined above. There are already plenty of rules that, when applied, restrict the usage of language capabilities that some people may find useful. It’s a matter of preference.

@ljharb
Copy link
Member

ljharb commented Mar 12, 2018

What would happen when you wanted to use the Foo component and widget in the same file?

Separately, i think that it’s not at all apparent that it’s more difficult to comprehend code when something has a different name in different files; the point of a module is that you think about one module (file) at a time.

@steve-taylor
Copy link
Author

steve-taylor commented Mar 12, 2018

What would happen when you wanted to use the Foo component and widget in the same file?

At that point, I'd consider naming things more appropriately. For example, I'd look at adopting a naming convention whereby widgets are suffixed by Widget (e.g. FooWidget).

As a last resort, there's always // eslint-disable-line: import/no-rename-default, but it's unlikely to be used when a naming convention is in place.

@steve-taylor
Copy link
Author

Let's also consider the following case (and I've been guilty of this before):

export default function getDefaultFoo() {
    // ....
}
import getMainFoo from '../foos/normal';

This can come about as my understanding of the scope, purpose and/or context of a function/class evolves while building it and using it for the first time. It would be nice to have a linting rule to remind me to clean this up by enforcing uniformity between its declared name and its imported name.

For those who don't see this as an issue, they can simply choose not to use this rule, like any other stylistic rule.

@steve-taylor
Copy link
Author

the point of a module is that you think about one module (file) at a time.

It would be nice if feature tickets and modules were nicely aligned. In reality (team of 5+ devs working on the same rapidly evolving codebase), implementing a feature typically involves reading, comprehending, and even modifying numerous modules. Hopefully you can appreciate that this proposal has come about based on a real dev team pain point rather than a contrived stylistic need.

@ljharb
Copy link
Member

ljharb commented Mar 13, 2018

It's also important to note that a default export need not have a name at all, consider export default () => {} or export default class { } - what would your rule do in that case?

Effectively this rule would disable a major feature of the language, the ability to rename imports (you're only requesting the default, but presumably it could apply to named as well via an option).

Perhaps you could share more about the pain points you hope to solve?

@steve-taylor
Copy link
Author

Effectively this rule would disable a major feature of the language

These rules also disable major features of the language:

  • import/no-anonymous-default-export - cannot export default () => {}, for example
  • no-default-export - no export default at all
  • import/no-unassigned-import - disables polyfills

Will they be removed?

what would your rule do in that case?

Ignore these exports. Use it with import/no-anonymous-default-export to ensure that never happens.

@ljharb
Copy link
Member

ljharb commented Mar 13, 2018

@steve-taylor fair point on existing rules that do that.

Can you explain more about the pain points? "it increases the difficulty of comprehending code" isn't something I've seen, on very large codebases, so I'd love something a bit more concrete to motivate this rule.

@steve-taylor
Copy link
Author

You’re smarter than me, then! The issue is that when there are a number of similarly named functions with a similar purpose that return the same type of object, but they are inconsistently named in default exports vs imports, it makes it more difficult than it needs to be to build a mental model of complex code. Same goes for classes. When one function or class has two names, it’s unnecessary noise, which is never a good thing when you’re reading code.

It’s hard to provide a less abstract example without handing over my employer’s code.

If you’re trying to figure out whether the effort is justified, I can save you most of the effort by submitting a pull request.

@ljharb
Copy link
Member

ljharb commented Mar 14, 2018

The effort I'm concerned about is future maintenance, not just the actual PR :-) Certainly it would be awesome if you were able to submit a PR if the request becomes accepted.

The readability issues you're talking about only apply when multiple of these similarly-named items are used in the same file? If so, wouldn't renames, in that file, actually help understand what the code is doing?

@steve-taylor
Copy link
Author

Not the same file. Spread throughout the same project.

@ljharb
Copy link
Member

ljharb commented Mar 14, 2018

I'm confused; when do you need a mental model beyond the file you're currently looking at, and its direct dependencies?

@steve-taylor
Copy link
Author

When I'm working on a feature/bug task that involves work across multiple files, which is almost every task ever. The use case is not a typical open source Node.js library that does one specific thing. It's a large web app or library, within a single git repo.

@steve-taylor
Copy link
Author

If you want to find all usages of a function or class throughout your project, and you don't have a fancy IDE that can find all usages regardless of name (e.g. WebStorm), a simple find-in-files is only going to work if all usages have the same name as each other and ideally the same name as the declaration.

@benmosher
Copy link
Member

Sounds useful to me. Future maintenance is always a factor but I don't think this would be a very substantial rule.

I think the implementation might be similar to no-named-as-default.

@ljharb, I hear what you're saying about disabling useful language features, but I try not to be too picky about style/preference/team strategy rules that folks express a desire for.

I also think I would probably use this one, would be great to know that all our React components are imported with the same name, as @steve-taylor describes. 👍

@benmosher
Copy link
Member

btw @ljharb I marked accepted but feel free to remove it if you still have concerns.

@omasback
Copy link

Is it really that hard to understand how it could be confusing for one thing to have many names? There's a reason most people go by the same first name their whole lives.

@ljharb
Copy link
Member

ljharb commented Apr 28, 2018

This would be the same rule as mandating that every time a specific function is called, you have to store it in a variable that’s named the same thing.

@omasback
Copy link

yeah exactly. I would like that.

@Dromin
Copy link

Dromin commented Jul 17, 2018

Situation I've run into in the past:

The name of a function / react component / feature changes; for example, let's say a function used to be called getAccounts() but is later renamed to getUsers(). The person making the change updates function name, filename, import paths, etc. but forgets to update the name of the imports in files where the function is used. (This actually happens pretty often if you use an IDE's automated refactoring functionality.)

Some time later another person is reading code that uses a function named getAccounts() and goes "Huh, that's weird. Our app doesn't even have accounts, we call those users... *scrolls up to the imports* import getAccounts from 'path/to/file/getUsers'; Oh..."

So yeah, it would be nice if eslint could catch default imports where the name doesn't match the name of the export; that way the person who made the change in the first place would immediately notice the problem.

@mic4ael mic4ael linked a pull request Jul 24, 2018 that will close this issue
@ljharb
Copy link
Member

ljharb commented Oct 19, 2018

Those listening: check out #1143; make sure it's what you might want.

@omasback
Copy link

It is a step in the right direction. I'm not sure it is even in scope for eslint to parse the imported modules and determine the correct name of the default export, which is the real desire. Sometimes the default export is not even named.

@threehams
Copy link

I briefly looked into this for my company, but naming collisions are a little too common when mixing in libraries. If you enable import/no-default-export that ensures that you get the original name by default and a rename only when it's necessary. If you're using TypeScript (or even vscode without it) you can get a bunch of other benefits from this, too: https://basarat.gitbooks.io/typescript/docs/tips/defaultIsBad.html

@FlorianWendelborn
Copy link

FlorianWendelborn commented May 19, 2022

If you're using TypeScript (or even vscode without it) you can get a bunch of other benefits from this, too: https://basarat.gitbooks.io/typescript/docs/tips/defaultIsBad.html

Updated link as of 2022: https://basarat.gitbook.io/typescript/main-1/defaultisbad

@steve-taylor
Copy link
Author

If you're using TypeScript (or even vscode without it) you can get a bunch of other benefits from this, too: https://basarat.gitbooks.io/typescript/docs/tips/defaultIsBad.html

Updated link as of 2022: https://basarat.gitbook.io/typescript/main-1/defaultisbad

Default exports aren't going away any time soon, though. There are frameworks that mandate them, such as Next.js.

Looks like this has been accepted, so I'll have a go at creating my first ESLint rule. Wish me luck!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment