Interfaces extending class with private props

[vidartf]

Describe the bug
When working on #6302, I tried to make an extension that would replace the document manager by providing the IDocumentManager token. So I make a new class that implements the interface, but immediately get hit by the error:

error TS2420: Class 'MyDocumentManager' incorrectly implements interface 'IDocumentManager'.
  Types have separate declarations of a private property '_findContext'.

It is not an alternative to not declare it, as it will then complain about the missing private property (!).

To Reproduce

Any interface declaration extending a class definition with private properties will throw this error, e.g.:

interface ISecrectInterface {};

class A {
  private _method(): ISecrectInterface {
    return {};
  }
}

interface IA extends A {};

class B implements IA {
  
}

Expected behavior
Exported interfaces, and especially token interfaces, should not include any private property declaration.

[vidartf]

Possible solutions:

• Use some TS magic to ensure interface only extends public properties of class.
• Maintain the interface declaration separately from the class definitions, i.e. stop using the pattern export interface IInterface extends CClass.

[vidartf]

Note: Could this be a bug in TS?

[jasongrout]

I totally agree that tokens should be exporting explicit interfaces, not interfaces derived from implementation classes.

[jasongrout]

Putting as 1.0 since a stable extension API is one of our goals for 1.0.

[vidartf]

TS magic:

type PublicShape<T> = Pick<T, keyof T>;

export interface IInterface extends PublicShape<CClass> {};

[jasongrout]

Pick only takes public attributes?

[vidartf]

According to the issue here: microsoft/TypeScript#31299

However, I am not certain if this is documented, or just "current behavior".

[jasongrout]

Ah, keyof currently skips private members. In this usecase, protected members probably also should be skipped?

I think it is much clearer if pure interfaces are the explicit public contract, and implementation classes always implement the token interfaces.

[vidartf]

Note: It seems super weird to me the interface picks up the private properties of the class, since declaring a private property on an interface gives a compile error. This is why I'm wondering if this is a TS bug.

[jasongrout]

Can you make a list of the token interfaces derived from implementation classes? I don't think this is a widespread problem - I think most places tokens are typed with pure interfaces.

[jasongrout]

This is why I'm wondering if this is a TS bug.

Sounds like a good issue to file with them.

[vidartf]

Xref: microsoft/TypeScript#471

[jasongrout]

microsoft/TypeScript#18499 has some relevant discussion as well.

[vidartf]

I think this is simply a quirk we will have to work around, given that publicShape is the conclusion in the xref issue I linked above. TL;DR: The issue is because the implements keywords can also be used directly between classes, and going via an interface seems to resolve down to this. I'm not going to bother to take this upstream at this point.

Note: Some places in the Lab code, this behavior is probably what we want. E.g.

export interface IShell extends Widget { ... }

Here, I think we definitely want to ensure that a replacement of IShell is a Widget with a solid identity to the known Widget.

[vidartf]

My conclusion here: We should only use the interface I extends C {} if we want to ensure that any implementer of I subclasses C.

[vidartf]

Current interfaces (mainly tokens) that should have a closer look:

• ILabShell (token)
• IThemeManager (token)
• ISettingsRegistry (token)
• docmanager/manager: Private.IContext (private type, so unsure if it matters)
• IDocumentManager (token)
• IRenderMimeRegistry (token)
• Builder.IManager
• NbConvert.IManager
• Setting.IManager
• Workspace.IManager

I don't know how the IManager pattern is generally used, so it might fall into either category (subclassing is required or not).

Recommendation: Extract the interface of all tokens, and revert the logic (interface declaration is explicit, and class implements that). This will add some friction when changing the interface of a Token, which is probably a Good Thing.

[blink1073]

IMO, I don't think we ever should explicitly expect a subclass at the token level, only potentially within our own class hierarchies.

[vidartf]

Current tokens that extends Widget (not guaranteed to be exhaustive):

• INotebookTools
• IShell
• ILabShell (extends LabShell which extends Widget)

[vidartf]

Note: I can do the legwork here, but I need input on:

• 👍 / 👎 on proposed solution for (non-Widget) tokens:

Extract the interface of all tokens, and revert the logic (interface declaration is explicit, and class implements that). This will add some friction when changing the interface of a Token, which is probably a Good Thing.

• What to do with the tokens that extends Widget.

[afshin]

ILabShell is very specifically meant to be the Lab application shell, so the fact that it extends LabShell seems okay to me. It's not the "generic Jupyter front-end" shell. What do you think?

[vidartf]

@blink1073 For the three Widget/token mixes outlined above, would you say that it makes sense to have the requirement that the provided object subclasses Widget? I'm guessing it would be a bit of work to have the shell not be a widget, and it seems deeply ingrained for INotebookTools as well.

[jasongrout]

I think it is okay to assume extends Widget should be a subclass. I think there is an explicit decision in phosphor design that you extend there by subclassing Widget, not implementing a Widget interface.

[blink1073]

👍 for what @jasongrout said

[vidartf]

I ended up adding the fix for this together with that of #6302 in #6334, as they are somewhat related.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related discussion.