Not only was TradeHelpers nuking other mod's trades a thing, it seems the API is also broken as well. Using the latest Fabric API, I noticed that villagers would show multiple entries for the same trade even though I only added it once.

In this case with a custom villager, calling TradeOfferHelper.registerVillagerOffers multiple times to add trades to each trade level will duplicate all the other trade levels each time. Specifically, the issue is caused by refreshVillagerOffers being called every time TradeOfferHelper.registerVillagerOffers is called by a mod.

Here is a more detailed look at what is going on using a debugger. I am calling TradeOfferHelper.registerVillagerOffers 5 times in order to add trades to each trade level. Show here is the entries in TradeOfferInternals.VILLAGER_TRADE_FACTORIES at the start of refreshVillagerOffers.

After the 1st TradeOfferHelper.registerVillagerOffers call:

After the 2nd TradeOfferHelper.registerVillagerOffers call:

After the 3rd TradeOfferHelper.registerVillagerOffers call:

Notice how on the second call, level 1 trades were duplicated. And on the third call, level 1 and 2 trades were duplicated again. And it keeps growing every time it is called making the trade map bloated like crazy. Someone suggested it is because it doesn't do a deep copy of the original vanilla maps it is suppose to replace. Instead it is feeding itself over and over.

However, a better solution for everyone is to trash the whole system and have the API just add/remove trades directly from vanilla's trade maps instead with none of these caching and reloadable system. It would both solve this issue and prevent the API from nuking other mod's trades when those mods do not use the API.

So it's indeed creating a new Map<Map<>> every time it refreshes the trade offers, so the outer map is copied but the inner maps are shared, leading to duplicate trade offers.

I am willing to fix this myself, most likely by just writing to the map directly like TelepathicGrunt suggested, but I have a question first (liach maybe?): What is refreshOffers? There is really no point imo, unless it re-runs the Consumer<List<TradeOffers.Factory>>, but that would be a breaking change I believe. I think we can remove its body and deprecate it without notice given how broken the API is!

Imo a way to fix this would be keeping the trade factories from our callback in a list than keeping original ones in a separate list. And Imo the factory creation should be lazy at best or reload performance would be horrible.

We should just write to the map directly imo.

Data-driven trades could then be removed from the map directly if necessary, for now it will require a mixin, and we can add an API for that later on.

Fixed via #1430, please reopen if issues persist.