[Discuss] Lint rule to enforce I prefix for typescript interfaces

[stacey-gammon]

We discussed using I prefix for typescript interfaces and decided to move forward with this as a convention. We just need to add a lint rule for it now.

[elasticmachine]

Pinging @elastic/kibana-operations (Team:Operations)

[mistic]

We can use the @typescript-eslint/interface-name-prefix
Is that supposed to be applied across the entire typescript codebase?

[timroes]

Will this also apply to interface representing (still) JavaScript classes? Like pure type definitions? Or just for real interfaces? Will it be used for all interfaces and interface-like types, or just interfaces? Are we planning to refactor the whole code base for that now (we already had 2 master breakages in the past week, because of type renaming).

[stacey-gammon]

Yep, the idea is to apply the @typescript-eslint/interface-name-prefix rule, so whatever that covers... just real interfaces, not type definitions.

Ideally, everything would be refactored across the codebase, if it's too big of an effort to do in one chunk, we could split it up by folder. Though as our code base grows so do these style change efforts. I'm starting to think about how we can keep this kind of thing up as we grow. Forcing one team to modify every file in Kibana to conform to a new lint rule is a lot of work, as is manually setting it up to do it folder by folder. Maybe we need some kind of versioned, published lint/formatting rules, that each plugin can bump when they are ready.

My main objective with this issue is to highlight the plan. E.g., you don't have to do this now, bc it's not yet a lint rule, but eventually, if you want to conform to Kibana's style guide, you'll have to do this.

[timroes]

Could you perhaps also clarify on the reason for this decision? The initial description unfortunately doesn't go into detail why we now want to use interface names? I just remember we had some discussions in the past, where we made an explicit decision not to use it, so it would be nice to read a bit more about the backgrounds here of the decision, please.

[stacey-gammon]

The benefits, imo, are to make the differentiation between what is a class and what is an interface clear and to enforce a consistent naming scheme.

Embeddables export IEmbeddable and Embeddable class. Using a lint rule means all interfaces vs classes have the same, consistent naming to differentiate them. It's obvious then to developers, when importing, to know if they are importing an interface or a class. Interfaces should be used, when possible, instead of a class, because it's less code being imported (I think that's accurate...).

So if you are writing a function that takes in something, like a filter manager, you can first look for the IFilterManager interface, instead of importing the FilterManager class, which you don't need. Depending on the use case, you might be able to avoid exporting the class at all, and only export the interface, plus the plugins.data.filterManager stateful instance.

Certainly open to arguments going the other way, especially since the original decision and discussion wasn't captured. There are quite a few arguments for the other side. Probably one of the strongest is that typescript/microsoft itself recommends against using this: microsoft/TypeScript-Handbook#121. You could also argue against my statement above and say that the plugin author should only export the interface and they should name it FilterManager so the user doesn't need to care that it's an interface. Then you have the issue of how to differentiate Embeddable vs IEmbeddable and users accidentally using the class not the interface. One possible way around this is to not export the class and instead export a factory service that returns the interface, that takes in something like createEmbeddable(defaults: EmbeddableDefaults): IEmbeddable.

https://stackoverflow.com/questions/31876947/confused-about-the-interface-and-class-coding-guidelines-for-typescript/41967120#41967120 has some interesting info too.

I don't know, I might actually be convincing myself out of this. If there is such a debate, why risk introducing a rule that we may eventually regret, when the effort to introduce the rule is large, and the benefit questionable. 🤔

[stacey-gammon]

Okay, so I finally found the slack discussion where this was loosely decided on back in July. Since slack is not a good place to track the reason for decisions, I would like to change this to a "discuss" topic, and give folks an opportunity to weigh in.

cc @elastic/kibana-platform @elastic/kibana-app-arch @epixa

I was originally for the rule, but after reading some of the reasons against, I'm starting to think it's not worth it to make this official.

[mshustov]

The platform teams decided on the I- prefix convention to separate interfaces and implementations only. That's TypeScript technical limitation that we had to work around to unblock our work during migration #33396
The real private fields microsoft/TypeScript#9950 should land in TS v3.8 in the mid. of February. When it's available, we can review our agreement and abandon the prefixing approach. Having said that, I don't think it makes sense to enforce this convention across Kibana.

[streamich]

I've worked on both types of TypeScript projects that explicitly prohibit prefixing interfaces with I and ones that explicitly require (like proposed here) you to use I with all your interfaces. There is nothing bad per-se to prefix interfaces with I, however, just as a warning wanted to note here that in general TypeScript community is shifting towards not prefixing with I and TypeScript core team itself recommends not to prefix interfaces with I. So, we are going against the trend here.

[stacey-gammon]

Thanks all for weighing in. Let's consider the matter closed with a decision of not moving forward with this as a linting rule, nor as a recommendation.