ardittristan / 5eSheet-resourcesPlus

A module that allows you to add more resources to 5e character sheets
GNU General Public License v3.0
9 stars 5 forks source link

Incorrect libWrapper usage #9

Closed ruipin closed 3 years ago

ruipin commented 3 years ago

Hi,

While investigating an incompatibility between this module and Inventory+ that occurs only when libWrapper is enabled, I've noticed you are using libWrapper incorrectly.

const originalGetData = game.dnd5e.applications.ActorSheet5eCharacter.prototype.getData;
if (typeof libWrapper === "function") {
    libWrapper.register(
    "resourcesplus",
    "game.dnd5e.applications.ActorSheet5eCharacter.prototype.getData",
    function (wrapper, ...args) {
        const sheetData = originalGetData.call(this);
        // ...
        wrapper.apply(this, args);
        return sheetData;
    },
    "WRAPPER"
    );
}

Notice how you call originalGetData instead of the first parameter of the method (wrapper in this case). You then call wrapper (otherwise libWrapper would complain, as you've chosen type "WRAPPER") but ignore the return value.

What is happening is that you execute before Inventory+, then Inventory+ wraps this method too. libWrapper guarantees you'll be called before Inventory+. However, because you're calling the prototype directly and ignoring the return value from wrapper.apply, you ignore all changes to sheetData done by Inventory+ (or any other module).

In essence, you're bypassing/ignoring the entire libWrapper mechanism, and will be incompatible with any other module that touches this specific method and ends up after you in the wrapper call chain.

A fixed version would call wrapper instead of originalGetData.

const originalGetData = game.dnd5e.applications.ActorSheet5eCharacter.prototype.getData;
if (typeof libWrapper === "function") {
    libWrapper.register(
    "resourcesplus",
    "game.dnd5e.applications.ActorSheet5eCharacter.prototype.getData",
    function (wrapper, ...args) {
        const sheetData = wrapper(...args);
        // ...
        return sheetData;
    },
    "WRAPPER"
    );
}

Also, note that the explicit syntax wrapper.apply(this, args) hasn't been necessary since libWrapper 1.0.0. Newer versions all support the simpler wrapper(...args).

ardittristan commented 3 years ago

fixed with c0650263c2644322d59510c69045edd51a63bd1b