ruipin / fvtt-lib-wrapper

Library for Foundry VTT which provides module developers with a simple way to modify core Foundry VTT code, while reducing the likelihood of conflict with other modules.
GNU Lesser General Public License v3.0
35 stars 15 forks source link

Support chaining more than once #6

Closed ruipin closed 3 years ago

ruipin commented 3 years ago

Issue

Currently, libWrapper explicitly prevents modules from chaining more than once:

libWrapper.register('my-fvtt-module', 'SightLayer.prototype.updateToken', function (wrapped, ...args) {
    wrapped(...args); // Works
    wrapped(...args); // Throws 'libWrapper: This wrapper function has already been called, and must not be called twice.'
}, 'MIXED');

This was implemented to avoid the extra complexities that come with managing the call chain as well as some libWrapper internal limitations (which have since been resolved, and thus are no longer the limiting factor).

Possible solutions

Option 1

It would be very simple to implement a situation where modules higher in the call chain do not see the multiple chaining, i.e. if we have 3 modules A, B, and C wrapping the same method in this order, and B chains twice:

A -> B -> C1 -> original
       -> C2 -> original

The biggest advantage of this option is that it is simply emulating the way that pure Javascript wrappers already work. This means there is no surprise for the user of the library, and things "just work".

This scales well enough if two modules chain twice. For example, if both A and B were to chain twice:

A -> B1 -> C11 -> original
        -> C12 -> original
  -> B2 -> C21 -> original
        -> C22 -> original

However, this could easily break module A or C. A might need to see every call to the original method, while C might not expect to see the same call twice. As such, it could end up causing some difficult-to-debug compatibility issues.

Option 2

It could be argued that the following would be more correct behaviour:

A1 -> B -> C1 -> original
        -> A2 -> C2 -> original

This would ensure A sees all calls to the original method, including those caused by multiple chaining. However, I suspect this actually makes it easier to break module A, as it breaches the principle of least surprise due to going against the way pure Javascript wrappers work.

This option also gets more complex if two modules chain twice. For example, if both A and B were to chain twice:

A1 -> B1 -> C111 -> original
         -> A2 -> C121 -> original
               -> C122 -> original
   -> B2 -> C211 -> original
         -> A3 -> C221 -> original
               -> C222 -> original

Option 3

Another possibility is to create a new type of override (e.g. MULTICHAIN) which must be unique (similar to OVERRIDE), and always comes between MIXED and OVERRIDE, i.e. it is always the penultimate call in the chain.

This override type would be allowed to chain the call twice, since there will no longer be modules between it and the final call in the chain, as such removing the need to manage the correct call chain.

The only significant disadvantage of this option is that, due to the uniqueness requirement, it prevents two modules from chaining the same wrapper more than once.

Conclusion

After going through these options, I am tempted to go with Option 1 since it is the most simplistic, and works most similar to pure Javascript wrappers. Option 3 is also tempting, but I think the strict limitation of only one module chaining twice is hard to justify purely on compatibility grounds.

ruipin commented 3 years ago

https://github.com/ruipin/fvtt-lib-wrapper/releases/tag/v1.0.7.0 now supports multiple chaining using Option 1.