[Accepted] [Done] Changes to the subscription model

[lokka30]

I am overall quite happy with how Treasury has launched, I was not expecting it to be as smooth as it is. Looking back, I think we made two mistakes in the library's fundamental design.
Unfortunately these changes will annoy the few developers who are currently using Treasury, but I think it is in the library's long-term interest to resolve. They should understand, and the current Treasury implementations we are aware of are very simple. Better to change things now, then when it will become so much difficult to later.

• Check this if resolved (be it confirmed or denied, but not pending).

---

Current votes:

• Immediately convert the APIs to use only CompletableFutures, removing the Subscriber
  • Voters:
• Immediately convert the APIs to use CompletableFutures by default, allowing a Subscriber conversion
  • Voters: @lokka30
• (Current) Use the Subscriber model by default but allow a CompletableFuture conversion
  • Voters: @MrIvanPlays @Jikoo @MrNemo64
• Pending: @yannicklamprecht @creatorfromhell

---

Problem: Subscription Model

Treasury has been out for a little over 3 weeks now. I have noticed the difficulty that developers face when they try to use our subscription model.

Ivan and I developed a simple 'set balance' method that would be commonly used, using both the subscription model and the Future model. The Future variant had 64% less lines of code, yet looked far cleaner, flowed better, and is much easier to maintain.

View Future code

public void setBalance(
    CommandSender sender, UUID targetPlayerId, BigDecimal amount, Currency currency
) {
    EconomyTransactionInitiator<?> initiator;
    if (sender instanceof Player) {
        initiator = EconomyTransactionInitiator.createInitiator(EconomyTransactionInitiator.Type.PLAYER,
                ((Player) sender).getUniqueId()
        );
    } else {
        initiator = EconomyTransactionInitiator.SERVER;  // better for it to be the plugin, or if the sender is null? /shrug
    }

    EconomySubscriber.<Boolean>asFuture(s -> economy.hasPlayerAccount(targetPlayerId, s))
        .thenCompose(val -> {
            if (val) {
                return EconomySubscriber.<PlayerAccount>asFuture(s -> economy.retrievePlayerAccount(targetPlayerId, s));
            } else {
                return EconomySubscriber.<PlayerAccount>asFuture(s -> economy.createPlayerAccount(targetPlayerId, s));
            }
        })
        .thenCompose(account -> EconomySubscriber.<BigDecimal>asFuture(s -> account.setBalance(
            amount, initiator, currency, s
        )))
        .whenComplete((bal, error) -> {
            if (error != null) {
                sender.sendMessage("Something went wrong!");
                return;
            }

            sender.sendMessage("Success! New balance is " + bal.doubleValue() + "!");
        });
}

(32 lines)

View Subscription code

public void setBalanceAsSubscriber(
    @NotNull CommandSender sender,
    @NotNull UUID targetPlayerId,
    @NotNull BigDecimal amount,
    @NotNull Currency currency
) {
    EconomyTransactionInitiator<?> initiator;
    if(sender instanceof Player) {
        initiator = new EconomyTransactionInitiator<>() {
            @Override
            public Object getData() { return ((Player) sender).getUniqueId(); }

            @Override
            public @NotNull Type getType() { return Type.PLAYER; }
        };
    } else {
        initiator = EconomyTransactionInitiator.SERVER;
    }

    economy.hasPlayerAccount(targetPlayerId, new EconomySubscriber<>() {
        @Override
        public void succeed(@NotNull Boolean result) {
            if(result) {
                economy.retrievePlayerAccount(targetPlayerId,
                    new EconomySubscriber<>() {
                        @Override
                        public void succeed(@NotNull PlayerAccount playerAccount) {
                            setBalance(playerAccount, initiator, amount, currency, (exception) -> {
                                if(exception == null) {
                                    sender.sendMessage("Success!");
                                } else {
                                    sender.sendMessage("Something went wrong!");
                                }
                            });
                        }

                        @Override
                        public void fail(@NotNull EconomyException exception) {
                            sender.sendMessage("Something went wrong!");
                        }
                    });
            } else {
                economy.createPlayerAccount(targetPlayerId,
                    new EconomySubscriber<>() {
                        @Override
                        public void succeed(@NotNull PlayerAccount playerAccount) {
                            setBalance(playerAccount, initiator, amount, currency, (newBal, exception) -> {
                                if(exception == null) {
                                    sender.sendMessage("Success! New balance is $" + newBal.doubleValue() + "!");
                                } else {
                                    sender.sendMessage("Something went wrong!");
                                }
                            });
                        }

                        @Override
                        public void fail(@NotNull EconomyException exception) {
                            sender.sendMessage("Something went wrong!");
                        }
                    });
            }
        }

        @Override
        public void fail(@NotNull EconomyException exception) {
            sender.sendMessage("Something went wrong!");
        }
    });
}

private void setBalanceAsSubscriber(
    @NotNull PlayerAccount playerAccount,
    @NotNull EconomyTransactionInitiator<?> initiator,
    @NotNull BigDecimal amount,
    @NotNull Currency currency,
    @NotNull BiConsumer<BigDecimal, EconomyException> consumer
) {
    playerAccount.setBalance(amount, initiator, currency, new EconomySubscriber<>() {
        @Override
        public void succeed(@NotNull BigDecimal bigDecimal) {
            consumer.accept(bigDecimal, null);
        }

        @Override
        public void fail(@NotNull EconomyException exception) {
            consumer.accept(null, exception);
        }
    });
}

(89 lines, harder to follow, 'bloated' looking, harder to maintain)

The entire purpose of the subscription system being the default was with the intention that it was easier to use. In fact, turns out it's harder to use, almost everyone will prefer the Future variant.

I propose that we make the methods use CompletableFutures by default. We could keep the subscription as an optional conversion if a developer does prefer using it, otherwise we could remove it entirely.

(Note: Nobody who worked on the Subscription system is at fault - we democratically decided to use it, without understanding the current realization of this outcome.)

---

Please post your thoughts below. Be raw, there is no need to put on a kind mask in this debate, but don't be rude. Provide adequate reasoning for your opinion if you'd like to share it.
I post this in hopes to improve the library, I am not suggesting that this change must be done.

Tagging users who may be interested: @MrIvanPlays @Jikoo @MrNemo64 @yannicklamprecht @creatorfromhell

[MrIvanPlays]

It is already too late to make breaking changes. We shall either do v2 and break stuff with also adding a lot of other features such as the permission api and other platforms support or leave it as is. There's no middle point.

[MrNemo64]

Rough decision, the api will take a hard hit in the short term but will probably be the best on the long term. Just seeing your example makes it obvious. The idea is to minimise the bloat but we failed, we should correct our mistakes.
I agree with @MrIvanPlays, I think we should do the break and give more APIs/platform support, as soon as possible

Don't put me on yes/no yet

[Jikoo]

Honestly the only reason the Subscriber model exists is because you thought futures were too complicated - this was a less complex (to understand, not to write!) alternative.

My thoughts on this were expressed pretty extensively in the PR for this (here, for instance: #37 (comment)). Futures are cleaner, but they make it easier for inexperienced developers to write low quality code. I wouldn't mind overloads (I know we had discussed and decided against them) but I do feel like the explicit failure handling and lack of ability to block the main thread for a return value are both important for making Treasury beginner-friendly.
We added the asFuture method so that more experienced developers could access the much simpler and cleaner approach without excess boilerplate.

Personally I think for advanced developers Futures are a better approach. The subscriber model was purely for accessibility and correct adoption of Treasury.

[lokka30]

Do you think we should just keep the system as-is (still use subscribers by default), but heavily suggest developers use Futures in the documentation? I was discussing this with Ivan on Discord earlier, I think he agreed with it.

[MrNemo64]

We can try an see how it goes
Explaining the reason as to why use futures and recommend them

[MrIvanPlays]

Keep as is and suggest futures if they know what they're doing

[Jikoo]

I think documenting and recommending proper Future use is the best way to go - if people read the docs, they will have nice, clean, comprehensible code. If they don't, we quietly attempt to railroad them into proper functionality.

[lokka30]

Sounds great, glad we're on the same page with this as well. I'll also mark this as resolved. I'll create an issue for improving the documentation.

[lokka30]

For future viewers, a resolution seems to have been made: in v2 we will be making a breaking change which switches to CFs as the standard rather than Subs.

[yannicklamprecht]

I'm on the CF train.