fix: align AlgorandClientInterface with AlgorandClient (#355)

[joe-p]

Proposed Changes

• Uses some TypeScript magic to keep AlgorandClientInterface aligned with the actual implementation
• Fixes AlgorandClientInterface is incomplete #355

[joe-p]

In 2a2a0ea I just completely got rid of the usage of AlgorandClientInterface and didn't seem to run into any problems. Presumably this was a problem with rollup at one point but no longer seems to be the case.

I do think it's worthwhile to refactor multiple areas of the codebase to use interfaces for better composability but right now this PR makes things a bit more consistent.

[robdmoore]

I was getting circular dependency issues when originally trying to use it directly.

[joe-p]

Are you still getting the errors locally? Building works fine for me locally and in the actions

[neilcampbell]

Running npx --yes madge src/index.ts --circular detects some circular dependencies. @robdmoore were you running madge or was rollup detecting them?

[joe-p]

These aren't true circular deps since it's just the types. b1451ff I've updated the imports to make that more clear. Out of the box madge doesn't handle import type correctly, but with some configuration it's now passing madge.

[neilcampbell]

@joe-p Yeah exactly.
Removing the interface is a breaking change, so I think we should keep exporting the AlgorandClientInterface and mark as deprecated. We could use your change in 2335219. The generated typed clients for example uses that interface.

[joe-p]

True, I can revert this PR to 5271138

[neilcampbell]

Yeah you could go either way, which do you prefer?

[joe-p]

I can revert and not mark as deprecated for now. In the future we can assess if there's opportunity for using interfaces in more places and then can refactor/deprecate accordingly.

[joe-p]

A negative consequence of using interfaces just occurred to me. Any addition to the AlgorandClient interface, such as a new method, becomes a breaking change. I think this means that using an interface solely to avoid circular dependencies is no longer a good enough reason to use interfaces and I'm leaning towards using import type { AlgorandClient }. Yes technically a breaking change but we are going to have a breaking change anyways with #371 if we stick with interfaces

[neilcampbell]

Any addition to the AlgorandClient interface, such as a new method, becomes a breaking change.

I don't quite understand how an addition to the AlgorandClientInterface would be a breaking change, but an addition to the AlgorandClient isn't?

[joe-p]

I don't quite understand how an addition to the AlgorandClientInterface would be a breaking change, but an addition to the AlgorandClient isn't?

Interfaces can be implemented by external libraries. For example, if I create an external class CustomAlgorandClient implements AlgorandClientInterface I am able to use that anywhere a AlgorandClientInterface is allowed. Once #371 is merged there will be a new method in the interface, registerErrorTransformer, which CustomAlgorandClient doesn't implement, thus it's a breaking change.

[neilcampbell]

Interfaces can be implemented by external libraries. For example, if I create an external class CustomAlgorandClient implements AlgorandClientInterface I am able to use that anywhere a AlgorandClientInterface is allowed. Once #371 is merged there will be a new method in the interface, registerErrorTransformer, which CustomAlgorandClient doesn't implement, thus it's a breaking change.

Ah yes of course, I wasn't thinking about using it as an input type.
I think it's probably highly unlikely that anyone is implementing their own AlgorandClient using the interface.

I agree I think the best path forward is to change back to import type { AlgorandClient }. To minimise the impact we should keep the AlgorandClientInterface (version in main) and mark as deprecated.

[joe-p]

I think it's probably highly unlikely that anyone is implementing their own AlgorandClient using the interface.

Agreed. we could make a semver exception because of this although I don't love doing that.

I agree I think the best path forward is to change back to import type { AlgorandClient }. To minimise the impact we should keep the AlgorandClientInterface (version in main) and mark as deprecated.

The problem is that marking as deprecated doesn't save us from making a breaking change since either we make a breaking change by replacing it or #371 is a breaking change. I think we just need to rip the band-aid off. The real question is whether we want to follow semver and increment MAJOR or not. I am typically against breaking semver, but since the chance of a single person implement the interface is fairly low perhaps this would be a justifiable instance.

[neilcampbell]

The problem is that marking as deprecated doesn't save us from making a breaking change since either we make a breaking change by replacing it or #371 is a breaking change. I think we just need to rip the band-aid off. The real question is whether we want to follow semver and increment MAJOR or not. I am typically against breaking semver, but since the chance of a single person implement the interface is fairly low perhaps this would be a justifiable instance.

Agreed, it's a bit of an annoying one. I feel that bumping the major may create more confusion (for likely no value as discussed above), given we're already supporting 2 versions in parallel.
If we don't bump the major we should keep the interface and deprecate, as the typed clients currently use it, so we'd want to maintain compatibility with any previously generated v4 typed clients.

@lempira Are you ok with not bumping the major here?

[joe-p]

If we don't bump the major we should keep the interface and deprecate, as the typed clients currently use it, so we'd want to maintain compatibility with any previously generated v4 typed clients.

Ah right forgot about typed clients, we'd need to do these update in lockstep then. Our options are either

A) Simply mark AlgorandClientInterface as deprecated, but don't replace its usage anywhere in the library meaning every addition to the interface is a breaking change and we'd need to continuously break semver when we add features to AlgorandClient (until next major release)

B) Replace AlgorandClientInterface with type { AlgorandClient } which would break the client generator, but we could then stick to semver going forward

We can't have best of both worlds (replace usage of AlgorandClientInterface with type AlgorandClient internally without affecting clients) because any AlgorandClientInterface output from the generated client would no longer work with utils inputs. So really it's

A) Continuously break utils semver until next release
B) Break utils semver once while also breaking semantic relationship between client and utils
or
C) Just follow semver and have breaking changes of utils and clients

The initial thinking of

I think it's probably highly unlikely that anyone is implementing their own AlgorandClient using the interface.

Doesn't apply because it would break the semantic relationship between clients and utils.

[neilcampbell]

We can't have best of both worlds (replace usage of AlgorandClientInterface with type AlgorandClient internally without affecting clients) because any AlgorandClientInterface output from the generated client would no longer work with utils inputs.

What if we used a type alias via export type AlgorandClientInterface = AlgorandClient instead? It means we're no longer exporting an interface, however I think that would solve the problem.

[joe-p]

What if we used a type alias via export type AlgorandClientInterface = AlgorandClient instead? It means we're no longer exporting an interface, however I think that would solve the problem.

Ah brilliant! Definitely hacky but I think it's the least amount of pain for everyone involved.

[lempira]

I would like major bump right now, but I also like to remain true to semver. I like the approach of export type AlgorandClientInterface = AlgorandClient for now while we wait on the ability to major version. Just to confirm, when we do bump major version in the future we are also going to deprecate the AlgorandClientInterface or remove it in favor of exporting the AlgorandClient?

[joe-p]

I like the approach of export type AlgorandClientInterface = AlgorandClient for now while we wait on the ability to major version.

Just want to clarify that this is still a breaking change and making this change without bumping MAJOR is breaking semver, but the affected users or libraries will be close to zero (and I think 0 is much more likely than > 0), especially with AlgorandClientInterface = AlgorandClient.

Just to confirm, when we do bump major version in the future we are also going to deprecate the AlgorandClientInterface or remove it in favor of exporting the AlgorandClient?

Correct, AlgorandClientInterface = AlgorandClient is essentially a hacky way to keep back compatibility with clients (or any other external library using AlgorandClientInterface). When clients updates its utils dependency, however, that will also be a breaking change that would call for breaking semver.

So this would mean we...

1. Use import type { AlgorandClient } instead of AlgorandClientInterface internally in utils and export type AlgorandClientInterface = AlgorandClient. These technically breaking changes to utils, but impact should be minimal.

2. Update clients to use latest utils. This will technically be a breaking change to clients, but impact should be minimal.

I also realized this would mean it's a breaking change for any lib that is using AlgorandClientInterface, including ones out of our control. For example, an external library might bump the version of utils and need to make the decision if they too want to break semver or not (and of course that's assuming they even realize that utils broke semver in the firstplace). Because of downstream impact, the more I think about it the more I'm inclined to say we should just folow semver and increment MAJOR. Yes a bit of PITA but I don't think we have full visibility into the downstream affects of breaking semver.

[lempira]

I would really like to avoid bumping a major version, given that just went through two major bumps in a relatively short amount of time. I would also like to remain true to semver so it doesn't lose meaning. Here are some suggestions

Options 1. - Extend the interface to a v2 interface

interface AlgorandClientInterface {
  // Base interface
}

interface AlgorandClientInterfaceV2 extends AlgorandClientInterface {
  // New functionality
  registerErrorTransformer(transformer: ErrorTransformer): void;
  unregisterErrorTransformer(transformer: ErrorTransformer): void;
}

Pro: Don't have to bump major version since we are not changing API
Con: Users have update to new interface to use functionality.

Option 2. Make the new functions in the interface optional. We could either do this by making the property optional with ?. If we did need it defined, we could have a default implementation. We could also type intersections to extend it

interface BaseAlgorandClient {
  // Pull existing functionality into a BaseAlgorandClient
}

interface ErrorTransforming {
  registerErrorTransformer(transformer: ErrorTransformer): void;
  unregisterErrorTransformer(transformer: ErrorTransformer): void;
}

// Users can implement just what they need
type AlgorandClientInterface = BaseAlgorandClient & Partial<ErrorTransforming>;

Pros: Done have to bump major version. Pretty simple option that lets user opt into that function be defining it.
Cons: The "extending through type intersection" method could be confusing because when you, you just want the client you don't want to know all the functionality to define it.

Option 3: Using a builder pattern. You have users define the errorTransformer with a new AlgorandClientInterface

//Change of current implementation
interface AlgorandClientInterface {
    // Current properties
    
    // error transformer builder method 'this' for after adding the errorTransformer to object
    registerErrorTransformer(transformer: ErrorTransformer): this;
    unregisterErrorTransformer(transformer: ErrorTransformer): this;
}

class AlgorandClient implements AlgorandClientInterface {
    private errorTransformer?: ErrorTransformer;


    registerErrorTransformer(transformer: ErrorTransformer): this {
        this.errorTransformers.add(transformer);
        return this;
    }

    unregisterErrorTransformer(transformer: ErrorTransformer): this {
        this.errorTransformers.delete(transformer);
        return this;
    }
}

//User implementation
const clientWithErrorTransformer = new AlgorandClient()
    .registerErrorTransformer(myTransformer1)
    .registerErrorTransformer(myTransformer2);

//unregister
client.unregisterErrorTransformer(myTransformer1);

Pros: No major bump.
Cons: I don't think manually adding this is ideal.

I think the simplest option is to make the new properties optional with defaults until a point when we bump the major version for major upgrade.

[joe-p]

I would really like to avoid bumping a major version, given that just went through two major bumps in a relatively short amount of time.

Why is there a strong aversion to MAJOR bump? It doesn't "feel" great but every other option so far feels worse.

Options 1. - Extend the interface to a v2 interface

The problem is that this doesn't solve the original issue in #355 unless we change usage of AlgorandClientInterface to AlgorandClientInterfacev2 which just brings us back to square one and requires a MAJOR bump according to semver.

Option 2. Make the new functions in the interface optional. We could either do this by making the property optional with ?. If we did need it defined, we could have a default implementation. We could also type intersections to extend it

This now means any optional functional may be undefined according to typescript. For example appClient.algorand.account.getAccountInformation will throw a TypeScript error and the developer will need to use appClient.algorand.account!.getAccountInformation. This also makes the type discovery/intellisense hard to navigate.

Option 3: Using a builder pattern. You have users define the errorTransformer with a new AlgorandClientInterface

This is how it's implemented now and has the problem of adding a method to the interface.

[joe-p]

After further discussion, there's been a consensus that the use of partial (as seen in 33f8755) is the best path forward. It avoids a breaking change while still giving developers the proper type, albeit some fields are erroneously marked optional. In the next major release the interfaces will be removed entirely and import type { AlgorandClient } will be used.