Move from final to @internal

[cleptric]

Problem Statement

The current code base marks most classes as final. This makes it almost impossible to people to extend our code if they have a valid reason to do so.

Solution Brainstorm

Moving forward (v4.0), we should switch to annotating our classes as @internal instead.

[cleptric]

One issue that I personally ran into was extending the Client, to be able to overwrite Client::addThrowableToEvent, so I can mark exceptions that get caught in my global framework error handler as handled: false.

[stayallive]

I would also very much like to see this. I know that this could bring maintenance burdens BUT I also feel like we are all adults in this industry and that the @internal annotation should sufficiently indicate the classes are for internal use only and that if you extend them you also know it could break at any time (also in bugfix releases) because they are not meant for external consumption.

I feel we should never force the user to not be able to do things (like extending our implementations because they are final) if they want to. Doing very little tweaks and changes to the SDK (sometimes temporary or for testing) is extremely frustrating because you need to implement a full range of classes and methods and can't use the default implementation without a full "copy all the code" which makes it very frustrating as a end user.

If a user chooses to extend a internal class and comes knocking that we broke their code it's very easy to point out they consented to doing it and help them fix it and move on.

[ste93cry]

I disagree for multiple reasons: first of all, obviously, the added maintenance burden. But more than that, final and @internal are two things that aren't opposite. @internal is used to indicate that something is not part of the public API, not that it should not be extended, while final prevents extending classes that are not meant to be extended. I recognize that it may make a lot of sense to open some of our classes to extension, no doubt about it, but I would also point out that composition is often the best solution instead of inheritance. I don't think we ever put a veto, if there was a valid case, to remove the final keyword from a class, but a lot of use cases simply don't need inheritance to be implemented and can just use composition: there are a lot of articles these days about why inheritance should be avoided as much as possible, and programming languages like Go don't even allow it, proof that is not considered a best practice in general.

[cleptric]

I want to avoid two things in v4.0

• Forcing people to update much of their code. This is a lot of work and very hard to justify to their managers.
• Changing the internals of the SDK too much. Dropping final is done quickly, adding/switching to composition APIs not soo much.

I'll propose some changes in a PR soon, maybe we can find a good middle ground here.

[ste93cry]

Forcing people to update much of their code. This is a lot of work and very hard to justify to their managers.

I'm not sure I get what this has to do with allowing people to extend classes. As soon as you make a class extendable, you are forcing people to update their code a lot more because they have to keep up with the changes you make, and in turn you can't make changes because you will break their code.

Dropping final is done quickly, adding/switching to composition APIs not soo much.

The current codebase is already favouring composition over inheritance. As I said, there are some cases where we may indeed want to allow people to extend the classes, but there are also many other cases where we don't want to. And honestly, I don't think that doing something just because it's quicker is a good way of thinking.

[cleptric]

I'm not sure I get what this has to do with allowing people to extend classes.

Nothing. I just wanted to make sure that we do not blindly rush into changing the whole API of the SDK to something composable and then let our users deal with it.

As soon as you make a class extendable, you are forcing people to update their code a lot more because they have to keep up with the changes you make, and in turn you can't make changes because you will break their code.

Can we make this assumption? So far, we didn't enable this for our users, so I don't think the minute we remove final that everyone blindly extends our classes. Again, @internal is a strong signal to people to expect BC breakages. Let them fix their issues in a temporary way.

The current codebase is already favouring composition over inheritance.

I added a very specific problem above. Let's start integrating on this.

[ste93cry]

Can we make this assumption?

There have been a few specific cases (mostly related to the part that initializes the transport and HTTP client) where people had some struggle in extending/changing the SDK, but apart from that I don't remember of people opening issues here and there to ask for more flexibility in this regard. I think we should guide our users to proper extension points, not let them shoot themselves in the foot and us with them.

Again, @internal is a strong signal to people to expect BC breakages. Let them fix their issues in a temporary way.

I highly doubt that people spend their day changing the behaviour of the SDK by extending classes and things like that. I can understand why you want to allow the Client class to be extendable, but I don't agree with your approach of just letting everything extendable just because "it's quicker" or based on the assumption that people need to change things (for which issues?), even more if there are no signs that this is needed for most of the things. I also want to remember that removing the final keyword is not a breaking change and so it can be done anytime, as soon as we find that we have a real need for it.

[Jean85]

I also want to remember that removing the final keyword is not a breaking change and so it can be done anytime, as soon as we find that we have a real need for it.

This. The contrary is a BC instead, so leaving those there is the conservative way to go.