-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[@typescript-eslint/naming-convention] Less strict handling of merged declarations #2223
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
Comments
I could see some limited value in adding a Making it work silently out of the box would be problematic IMO, as it means that unintentional name clashes would silently be ignored - which may cause inconsistencies in your codebase, and will confuse people: const example = 1;
type example = string | bool;
// OOPS - didn't upper case the E!
// But there won't be an error due to it automatically being detected as a merged declaration Part of me thinks that this case might be rare enough that the implementation complexity, user configuration complexity, and potential silent weirdness might not be worth it; instead it might just be better to use a disable comment. A disable comment has the added bonus of being very clear and explicit and showing exactly why you've broken naming convention in this case. // ('typeLike' must have 'PascalCase')
export interface Example2 {
x: boolean;
}
// eslint-disable-next-line @typescript-eslint/naming-convention -- intentionally breaking naming convention for declaration merging with the type
export function Example2(): Example2 {
return { x: false };
} |
If it helps, here is a real world example that encountered this problem:
Sample usage: The ApiPackage class extends from export class ApiPackage extends ApiItemContainerMixin(ApiNameMixin(ApiDocumentedItem)) {
public constructor(options: IApiPackageOptions) {
super(options);
}
. . .
} All of the mixins in that project rely on merged declarations in this way. |
🤔 Why doesn't |
The rule was inspired by this tslint rule, which didn't have a selector for it. And because I rarely ever use namespaces, I may or may not have completely forgotten about them during the initial implementation..... 😅 With the concrete examples, I had a play around with it and to clarify "declaration merging" is half right. You've got both declaration merging and you've also got variable shadowing across scopes (type scope / value scope). declaration merging what's happening to join the function and the namespace together. Both cases have this problem, however. if you merge a namespace into a function, they need the same name. I'm still not sure if this should be built into the rule or not. The problem is that adding a modifier for the thing doesn't really work, because how do you configure it? For example, if you want to configure the rule to allow an interface to shadow a variable or a function, but variables require Is there one modifier per shadow case - There's also the question of how you validate a merged/shadowed name? You can't just ignore it because otherwise you could do weird things like name a variable with snake case and an interface with snake case, but it'd be all okay purely because of the shadowing. So which format do you use? The format for the one that appears in the source code? Do we add configuration for a "priority" order for picking the format (i.e. variable before interface before namespace...) The more I think about it, the more I think that a disable comment is the best solution. |
It's true that the type system embodies a runtime "value" and a compile-time "type" that coexist. But I'm not sure there's any meaningful distinction between "merging" value+value (e.g.
Above, I proposed a simple and well-defined answer: Collect all the declarations that have the same name, test each of their In other words, we accept the name for a |
There's a slight difference in the way the two work - shadowing can work regardless of the scope or file, but you can only merge things declared in the same scope in the same file. Eg; you can shadow any imported type with a local variable, or a variable from an upper scope with a type in a lower scope. OTOH, you cannot merge an imported class with a local namespace, nor can you merge a class declared in an upper scope with an interface declared in a lower scope. This means there is an important difference in the way these two are handled, because it means that each style can be applied to certain constructs in certain ways
Declaration merging is simple enough for most cases:
Shadowing is where things get more complex, and I think we should ignore it completely. Declaration merging is an intentional thing IMO, and thus it's easy to detect and deal with Note that doing any form of work on based on the scopes will require the scope manager work (#1939), which is still a ways off. |
I never noticed that! Agreed, it is a significant difference.
Hmm... In the example that I gave, the mixin These cases are exotic enough that perhaps the lint rules should simply ignore them: A person who declares a weird merged type is already doing something relatively advanced, so maybe we can trust that they are smart enough to capitalize the name properly. |
Repro
TypeScript allows two different declarations to use the same name; they get merged into a single type. Besides a small handful of useful scenarios (e.g.
namespace
+enum
), merged declarations generally arise as hacks in advanced abstractions (e.g. mixins) or retrofitted types for a legacy API. To avoid these complexities, I'll give somewhat contrived examples below.The problem is that
@typescript-eslint/naming-convention
seems to check each declaration independently, whereas merged declarations must all have the same name. As a result, ESLint may reject every possible name.A couple example inputs:
Expected Result
Here's a better way to handle merged declarations: The
@typescript-eslint/naming-convention
rule should recognize merged declarations, and accept ANY applicable pattern, instead of applying ALL patterns.For the sample declaration
Example2
above, ESLint could accept EITHER 'PascalCase' OR 'camelCase' (whereas currently it requires BOTH).Versions
@typescript-eslint/eslint-plugin
3.3.0
@typescript-eslint/parser
3.3.0
TypeScript
3.5.3
ESLint
7.2.0
node
12.17.0
npm
6.14.4
The text was updated successfully, but these errors were encountered: