Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

tslint:recommended interface-name uses always-prefix #1520

Closed
spiffytech opened this issue Aug 26, 2016 · 12 comments · Fixed by #4871
Closed

tslint:recommended interface-name uses always-prefix #1520

spiffytech opened this issue Aug 26, 2016 · 12 comments · Fixed by #4871

Comments

@spiffytech
Copy link

tslint:recommended defaults to always-prefix. However, the authoritative sources I can find all discourage this behavior.

I cannot find documented rationale for tslint's choice to recommend always-prefix despite being at odds with the broader community's practice.

Please document the rationale for this, and/or consider changing the default to never-prefix.

@nomaed
Copy link
Contributor

nomaed commented Aug 28, 2016

@spiffytech Although there are style-guides that don't recommend using the I prefix, from my experience, it's not necessarily true that "the broader community's practice" is to follow these recommendations.

When the interface is used purely for typings of external modules (Window, HTMLElement, etc), it makes sense not to use the prefix since you're describing existing functions, objects and classes using interfaces. So you're not REALLY actually using interfaces in the purest sense.

However, when writing your own code base, the interfaces often have a different semantic meaning in which they actually define an an API to be follow, a contract without any implementation.
In these cases, you might have an interface named IConfiguration and a class named Configuration that implements it, and maybe even child classes that extend it and implement this same interface.

I think that when writing a library using TypeScript from scratch, this is the more common use case.

@jkillian
Copy link
Contributor

It also can be helpful to have interfaces visually distinguished when looking at import statements: imports that only import interfaces are elided by the TS compiler. This can affect your code if the module import would've had some other side-effect.

I'm somewhat ambivalent overall though

@simonbuchan
Copy link

I find the vast majority of my uses of interfaces are to describe (possibly extended) objects: eg options, react props, JSON config files, REST responses, etc. Using a prefix in this case is quite confusing.

Should I instead be using type Name = { ... }? I pretty much never see that in existing code.

I get the feeling that if you have a lot of IFoo and Foo, then you might be writing C# in typescript, and there is a strong push in the wider JS community to avoid that style of "classes for everything" - eg. react stateless components.

If I had to pick "always" or "never" I'd lean towards "never", but at the moment I'd say it depends on the code base too much. Really, I would say the recommended rule should be something like "only-with-matching-class" - eg exported IFoo if and only if there is also an exported class Foo.

It would be interesting to see an analysis of open typescript code to see if there is a strong existing preference though.

Something to note, the typescript compiler itself seems to generally uses public interface Foo and one or more createFoo() that return either an internal class FooImpl, or an extended object.

@frosas
Copy link

frosas commented Nov 23, 2017

FYI, TypeScript contributors are also asked to not prefix interface names: https://github.com/Microsoft/TypeScript/wiki/Coding-guidelines

@lukescott
Copy link

Instead of picking "always" or "never" as default, can you just remove it from recommended? As far as I know the I prefix is really only used a lot in .NET (and Java). I think a lot of people assume I is best practice because TypeScript comes from Microsoft. Which, in my experience, most TypeScript code that I've used/write do not do this at all.

@redstrike
Copy link

I'm patching winston v2.x type definition file to make the type checker feels happy with the actual winston v3.x in node_modules.

I like the I prefix convention in C# or Java world and eager to adopt it. However, most of popular JS libraries are not following this convention. So, it feels like I'm staying away from the active communities of JS world.

IMO, we should remove this rule from tslint:recommended. It's holding me back while developing, then I reached this issue :(

@sumowrestler
Copy link

The Java JDK libraries do not use Hungarian notation, see lang packages etc...
https://docs.oracle.com/javase/8/docs/api/overview-summary.html

Some good reasoning against here:
http://www.richardlord.net/blog/actionscript/the-i-in-interface.html

Not a fan of it, and having it required as a default encourages developers to stay away from interfaces all together so I would argue there's a strong case for never-prefix as default.

@JoshuaKGoldberg
Copy link
Contributor

FWIW, reasoning in favor of always is that when you have a single class in source code and stubbed equivalents in tests implementing an interface, what do you call the class compared to the interface?

interface INameValidator {
    validate(name: string): boolean;
}

class NameValidator implements INameValidator {
    public validate(name: string): boolean { /* ... */ }
}

@lukescott
Copy link

lukescott commented Nov 3, 2018

I would call my interface ‘Validator’ as my implementation is usually specific and my interfaces are more abstract. But these interfaces are typically for actual use, not testing. With testing you don’t have to formally declare an interface to test it.

As I said, I would rather ‘recommended’ not take a stance on it either way - neither always nor never. Just remove it. This is a personal / organization preference, not a best practice.

@JoshuaKGoldberg
Copy link
Contributor

Agreed, let's get this out of recommended. 😊

@JoshuaKGoldberg
Copy link
Contributor

Related superset issue: #4305

@JoshuaKGoldberg
Copy link
Contributor

Removing the Type: Breaking Change label per #4811. Now accepting PRs!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants