Make TspClient reuseable interface

[sgraband]

The TspClient is now an interface that can be used for other implementations. The existing functionality was moved to the DefaultTspClient. This means adopters now need to initialize the DefaultTspClient type instead of TspClient to get the default behavior. However this makes the package more flexible as different implementations can be provided, e.g. a proxy client in a Theia frontend.

Part of eclipse-cdt-cloud/theia-trace-extension#976

Contributed on behalf of STMicroelectronics

Note: Once this is merged i can update this PR to consume the changes.

[marco-miller]

Thanks for this enabling contribution yes. So these would be my few (minor) comments.

Now this PR would require extensions [1,2,3] adaptations, so they build and work upon using it. Would you be able to contribute companion PRs for those alongside this one, accordingly? The goal would also be to further test this very PR that way, making sure that it works in [1,2,3]'s own contexts in turn. The original goal of supporting [4] remains alongside.

The concern of potentially breaking other (unforeseen) client integrations remains, though. Even if likely only a matter of renaming the type; as this has been a published API for a while now. Is there something we could do for this, say, scaffolding a backward-compatible (yet lightweight) structure first? -Which would come with a deprecation plan or so.

[1] https://github.com/eclipse-cdt-cloud/theia-trace-extension
[2] https://github.com/eclipse-cdt-cloud/vscode-trace-extension
[3] https://github.com/eclipse-cdt-cloud/vscode-trace-server
[4] eclipse-cdt-cloud/theia-trace-extension#990

[sgraband]

@marco-miller I updated the PR according to your two feedback comments. I will also provide PRs in the coming days for the dependedent projects.

Regarding the breaking of other dependent projects: I think the main problem tha we have is that the new interface has the same name as the old constructor. Hence exporting a constructor for TspClient will not work if i am not mistaken. Do you have another idea on how to achieve this?

BTW, i feel like it makes sense to get this PR merge ready, before updating the PRs in the dependent projects in case we change something here.

[marco-miller]

@marco-miller I updated the PR according to your two feedback comments. I will also provide PRs in the coming days for the dependedent projects.

Thanks again for all this.

Regarding the breaking of other dependent projects: I think the main problem tha we have is that the new interface has the same name as the old constructor. Hence exporting a constructor for TspClient will not work if i am not mistaken. Do you have another idea on how to achieve this?

We could consider keeping the legacy constructor (class) as is. While making it extend a new, differently named interface, behind the scenes -so to speak. Then add the new constructable sibling alongside that legacy one (extending that interface too). Existing clients wouldn't be broken, yet could eventually choose to switch to the new implementation.

BTW, i feel like it makes sense to get this PR merge ready, before updating the PRs in the dependent projects in case we change something here.

Agree; makes sense to me too.

[sgraband]

We could consider keeping the legacy constructor (class) as is. While making it extend a new, differently named interface, behind the scenes -so to speak. Then add the new constructable sibling alongside that legacy one (extending that interface too). Existing clients wouldn't be broken, yet could eventually choose to switch to the new implementation.

Would we really provide a "separate implementation" for the old constructor? Wouldnt it be sufficient to do something like:

export class TspClient extends HttpTspClient {}

This way we do not need to provide another implementation or delegate and existings clients will not break. WDYT?

Do you have any suggestions for the name of the interface? Are we planning to rename it to TspClient once we deprecate the constructor or should the name be permanent?

[marco-miller]

We could consider keeping the legacy constructor (class) as is. While making it extend a new, differently named interface, behind the scenes -so to speak. Then add the new constructable sibling alongside that legacy one (extending that interface too). Existing clients wouldn't be broken, yet could eventually choose to switch to the new implementation.

Would we really provide a "separate implementation" for the old constructor? Wouldnt it be sufficient to do something like:

export class TspClient extends HttpTspClient {}

This way we do not need to provide another implementation or delegate and existings clients will not break. WDYT?

Agree. I'd in fact be for the less breaking and most stable (yet still fitting) solution possible here. So, as far as HttpTspClient is as stable and predictable as TspClient was known to be.

Do you have any suggestions for the name of the interface? Are we planning to rename it to TspClient once we deprecate the constructor or should the name be permanent?

How about TraceServerProtocol? But I'd prefer that @bhufmann shares his opinion as well here. I'd foster not having to rename the interface anytime soon, even after eventually deprecating the legacy constructor.

[paul-marechal]

export class TspClient extends HttpTspClient {}

No use creating a child class when all you want is an alias:

/** @deprecated use {@link HttpTspClient} directly instead. */
export const TspClient = HttpTspClient;

[paul-marechal]

Do you have any suggestions for the name of the interface? Are we planning to rename it to TspClient once we deprecate the constructor or should the name be permanent?

How about TraceServerProtocol?

If I may, I'd throw TraceClient for consideration because the way the interface is defined it's all methods to interface with some Trace Server and it seems rather protocol-agnostic?

[bhufmann]

Do you have any suggestions for the name of the interface? Are we planning to rename it to TspClient once we deprecate the constructor or should the name be permanent?

How about TraceServerProtocol?

If I may, I'd throw TraceClient for consideration because the way the interface is defined it's all methods to interface with some Trace Server and it seems rather protocol-agnostic?

Finding good names are often tricky. TraceServerProtocol feels a bit long and TraceClient doesn't have a clear meaning for me.

With this change TspClient is an interface. The name doesn't indicate that. In Java, I like the naming convention that interfaces start with I. But this is discouraged in the Typescript community. So, I'm ok with keeping TspClient for the interface. HttpTspClient then makes sense for the HTTP implementation.

[bhufmann]

Do you have any suggestions for the name of the interface? Are we planning to rename it to TspClient once we deprecate the constructor or should the name be permanent?

How about TraceServerProtocol?

If I may, I'd throw TraceClient for consideration because the way the interface is defined it's all methods to interface with some Trace Server and it seems rather protocol-agnostic?

Finding good names are often tricky. TraceServerProtocol feels a bit long and TraceClient doesn't have a clear meaning for me.

With this change TspClient is an interface. The name doesn't indicate that. In Java, I like the naming convention that interfaces start with I. But this is discouraged in the Typescript community. So, I'm ok with keeping TspClient for the interface. HttpTspClient then makes sense for the HTTP implementation.

"TspClient" as interface name and "HttpTspClient implements TspClient" would make sense. For backwards-compatibility reasons and to avoid having to update each user of TspClient (theia-trace-extension and vscode-trace-extension) right away we don't have a choice to find a new name for the interface. Maybe using the prefix I is not such a bad idea in this case. I have seen that being done in the actual VsCode Typescript code base. So we wouldn't be the only ones :-)

WDYT?

[marco-miller]

Adding to @bhufmann's latest proposal (which I'd foster, along with @paul-marechal's alias one): the exact list of known client extensions not to break is [1,2,3] in this previous reply. -Just a friendly reminder.

[sgraband]

Thanks a lot for the suggestions! I now went with ITspClient for the interface the same naming scheme is already used for the ITspClientProvider in the theia-trace-extension.

I tried using a simple alias, which worked for the constructors, but not for the types, due to:

error TS2749: 'TspClient' refers to a value, but is being used as a type here. Did you mean 'typeof TspClient'

So instead i went for the class ... extends way, which works.

I also tested it with [2] and [3] and there seem to be no breakages. I am currently working on extending eclipse-cdt-cloud/theia-trace-extension#990 to work with these changes.

[marco-miller]

Thanks again for the rework. At this stage, most of the fix-up commits could be squashed, as they're review-based steps more than commitable increments. The resulting commit message(s) can be confirmed alongside.

[marco-miller]

Pending @bhufmann's 2nd review; thanks.

• I yarn-linked this client version into the aforementioned [1,2,3] and was able to build and run tests where available.
• I opened the files using TspClient and didn't notice new/related warnings in VSC.
• The HttpTspClient implementation looks identical to the legacy TspClient one to me locally.

[bhufmann]

It looks good to me. Works fine with theia-trace-extension and vscode-trace-extension.

Thanks for the contribution!

[MatthewKhouzam]

Nice modularity!