Kibana: Style Vote: Interfaces vs Duck typing

[stacey-gammon]

If we have a common structure that an object should abide by, is relying on duck-typing alone sufficient, or should we be using an interface for better documentation?

Background: I'm introducing the concept of EmbeddableHandlers to make it easy to create plugins for new dashboard panel types, and have them be renderable. Every EmbeddableHandler must abide by a certain structure and provide certain functions that Dashboard can call (e.g. render).

I can rely on duck typing and fall back to jsdoc to document for what the structure should look like, but it feels like creating an interface provides clearer documentation and is easier to find. You can even do something like

class EmbeddableHandlerInterface {
  render(element, panel) {
    throw new Error('Must implement render method');
  }
}

for run time error messaging.

One of my issues with relying on jsdoc's @typedef is that there isn't always an obvious location to put it in that is included by all the implementors. Should we create a new file with only comments in it and include that?

I also find it more likely a comment like @type {EmbeddableHandler} will be glossed over/ignored, rather than class SavedSearchEmbeddableHandler extends EmbeddableHandlerInterface

From what I can tell, in most situations in our current code we rely on duck typing with little to no documentation. I just want to verify this is the way we want to go before I follow suit.

Case Study: EmbeddableHandlers: https://github.com/elastic/kibana/pull/1214
Additional Case Study: Make all return values for checkLicense share a common structure: https://github.com/elastic/x-pack-kibana/issues/1628

I spied some typescript in the new experimental platform code, and typescript provides great support for interfaces. If the new platform is indeed going that route, perhaps starting to migrate in that direction is not a bad idea.

👍 if you are in favor of using interfaces, 👎 if you think relying on duck typing alone is sufficient, and please comment if you have other ideas!

[uboness]

IMO an "interface" or in TS lingo - type definition, provides better documentation for the expected types. Plus... in the future we might want to use some type checking as part of the build process and having type definitions around seems like the only way to go about it (more on this to come later.. though you already hinted it out ;)). Duck typing is a cool hack that, IMO, fails to scale to the level we need it to in Kibana.

+1 on throwing a clear error message by default

[chrisronline]

I think interfaces make a lot of sense and we don't really lose anything by using them. The clear error message is a big win that you don't have in a duck typing environment where someone forgets to check if something exists.

My only concern with extends ... is getting into a situation where we can't take advantage of the compositional power of javascript, but it seems there is a way to handle that already

[kobelb]

@chrisronline agreed! There's a reason why most OO languages don't support multi-inheritance, and it's a good argument for using Composition over Inheritance and using an interface here once we have such facilities. cough TypeScript cough

[Bargs]

+1 on types/interfaces, but please please please don't use classes to implement them. I'd much rather wait for typescript or any other solution that doesn't break encapsulation by exposing a module's object instantiation strategy via the new keyword. I'd also really like to be able to implement multiple types without resorting to hacks.

If the main desire here is for a place to store a canonical description of the interface and validate that implementors follow that interface, we could even use something as simple as Joi.

[stacey-gammon]

Perhaps we need another vote issue for how to implement interfaces in the interim, before we go the typescript route (if we do indeed do that). Classes/extends vs Joi? Using classes seems like the conversion to typescript will be easier, but am open to other ideas like Joi.

@epixa - You mentioned @kjbekkelund will be sharing some information regarding Typescript with the team soon. Might it help us avoid these "poor man's interface" solutions?

[thomasneirynck]

@Bargs I don't think this proposal is about instantiation. That can still happen with e.g. a factory-function that hides the implementation details. This is more about writing a self-documenting code-base. Using the built-in JS features for that. Types often have is-a relationships, and class and extends do an adequate job to describe that IMO.

Also the word class is just a name. It's sugar for defining a prototype in a concise manner. It doesn't mean that we need to use all the "bad"-OO patterns that are associated with this.

The problem with using new for instantiation is mainly when OO-libraries expose a public API. The new requires 3rd party users have exposure to a specific implementation, that the library-maintainer may want to alter later on. That is is less of a problem in Kibana, because (1) 90% of it is a closed code-base where we have control over both the "library"-code (ie. the modules that define the types) and the "client"-code (ie. the dependencies that create these types). Also (2), JS is not a compiled language where compilation errors would arise very easily with even the slightest change to a class. This is much less the case in JS, since it's perfectly possible to make changes to types (extending it, changing its prototype chain), with less risk of breaking any dependencies.

[Bargs]

I don't think this proposal is about instantiation.

My point exactly. Using types shouldn't force you to use a specific instantiation strategy. If we use classes to define types, they will. Sure, you could wrap every single "type implementation" in a factory function, but that's a pointless extra layer of complexity when the factory function could just create the object directly. No one is going to create that wrapper.

This is more about writing a self-documenting code-base. Using the built-in JS features for that. Types often have is-a relationships, and class and extends do an adequate job to describe that IMO.

No, classes aren't a built-in JS feature for creating types. Classes make very poor types. extends will only give us errors when an un-implemented function is actually called at runtime. extends doesn't allow us to implement multiple types without hackery. Classes provide no way to program to an interface and select an implementation polymorphically. typeof classInstance doesn't work. And using classes as types means only other classes can implement types.

(1) 90% of it is a closed code-base where we have control over both the "library"-code (ie. the modules that define the types) and the "client"-code (ie. the dependencies that create these types).

We're working towards providing public API. If we use classes as types, we will end up leaking implementation details to third parties and then we'll regret it.

(2), JS is not a compiled language where compilation errors would arise very easily with even the slightest change to a class. This is much less the case in JS, since it's perfectly possible to make changes to types (extending it, changing its prototype chain), with less risk of breaking any dependencies.

This is not the problem I was referring to. The issue is that if you use new, you're stuck with it. What if you decide you want to turn your class into a factory function that caches the object instances it returns? Too bad, all of your client code is already calling new. (FWIW I also don't buy that changes to a public API are less risky just because JS is dynamic. You're just trading compile time errors for subtle runtime errors you'll hit in production).

Sure, you could hack around all of these problems if there was some massive benefit to using classes, but there isn't.

[epixa]

I'm not sure if there is a definitive rule we can adopt with this one as the requirements of every API are so specific to that API. It might make sense for the public API of an "embeddable" plugin to include a base class, just like you get with a declarative component-based system like react, but it might not make sense to do the same for the API to create kibana query language queries.

Typescript doesn't provide a complete answer here, either, since our plugin API will have native JS boundaries. With typescript, we'll be able to use static typing/interfaces when developing within any given system in Kibana, but the public APIs we expose in our systems will be runtime APIs, so the checks we do on data will need to be runtime checks.

[stacey-gammon]

Final decision: we are moving to typescript so will have support for interfaces. Closing as this is no longer an active discussion.