-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Shim does not support instance-specific / inherited-method wrapping. #3
Comments
What makes implementing this complicated? (I'm thinking of taking a stab at this). |
Sorry for the delay in answering. I was going to reply yesterday, but was busy with some other things and wanted to find a better time to sit down and write an in-depth answer, instead of rushing through it. 1. Is it necessary?The original use case of libWrapper was to transparently replace the standard way of monkey-patching, e.g. let old = method.to.wrap;
method.to.wrap = function(this, ...args) {
return old.call(this, ...args);
} into libWrapper.register(module, "method.to.wrap", function(this, next, ...args) {
return next(...args);
}); The shim was meant to support this use-case only, allowing someone not interested in more advanced features to use libWrapper when available, and have no functional change when not. It is my opinion the shim should be as small as possible, to avoid the situation where people are essentially bringing in most of libWrapper into their own module for what was originally meant to be just a "thin" and "simple" compatibility shim. Adding these advanced features (instance, and inherited wrapping) risks significant feature creep. 2. Is it useful?I don't know of any module that uses instance-specific or inherited-method wrapping without libWrapper. I suspect this is because while there are some use cases, doing this can have significant issues. One could argue here that "they don't use it because it's hard to", and that libWrapper making it easier might mean people would be more likely to use it. But I have my doubts. I don't disagree that there are use-cases. I have yet to find a convincing real-world one, but if such a use-case existing, nothing is stopping users from requiring libWrapper, or implementing this functionality themselves. 3. Is the implementation cost too big?While I added support for these to the full libWrapper library mostly as a bug fix (they "worked" in most cases, but caused bad behaviour in some others, which breached the principle of least surprise), the libWrapper library already had a lot of existing code to cater for this which made supporting this functionality much easier. The requirements of such a system should be functionally mostly the same as the full library. You can see the corresponding unit tests here: fvtt-lib-wrapper/tests/test_wrapper.js Lines 151 to 252 in 4310380
Note that these tests are likely not complete, but they should cover the most common situations I could think of. I wouldn't be surprised if in more complex inheritance cases, there were still bugs to be found. In particular, an instance-specific wrapper needs to be able to call an instance-specific method, if it exists, and if not then it needs to call the class method. A wrapper on an inherited class needs to check the class first, and if the wrapped method isn't in the current class, it needs to "climb the hierarchy" to find the method to call. Because these can change dynamically (the user might wrap the instance, then wrap the class method, then wrap the inherited class, in this order), this can only be achieved using dynamic dispatch, and can't easily be resolved statically at wrapping time. It is important to note that these can change dynamically. As an illustrative example, a user might wrap the same method three times, in the following order:
The first wrapper ("Instance") would need to first wrap the parent class, then wrap the parent class wrapper, then wrap the child class wrapper, without being reregistered. As such, dynamic dispatch becomes a requirement. The full libWrapper library already relies on dynamic dispatch for all its wrapping, since the wrapper list is allowed to change at any time. The wrapper methods the user provide will themselves be wrapped by libWrapper code which handles dispatch to the next level of the call chain, possibly the instance class, or a parent class for an inherited method. As such, it was relatively simple to implement this capability. Refer to the changes to wrapper.js in the relevant commit: The shim has none of this dynamic dispatch infrastructure, and I believe it is likely overkill to implement it without even having a real-world use-case to point to. There is an option of having the shim do static dispatch, i.e. you figure out which method needs to be wrapped at wrapper registration time, and assume that will never change, documenting this assumption. This means the user musn't wrap things in the wrong order, or things break. Going back to the example above, they would need to wrap starting at the bottom of the call chain:
If they did the original order, the "Instance" wrapper would skip the other two wrappers, as it would have been registered earlier. Is this an acceptable limitation? Perhaps. Is it worth it? Not sure, especially since this would likely already involve a significant amount of extra code (the shim is very "thin" and "simple" on purpose). 4. ConclusionI am undecided on whether this is worth it or not. I remain unconvinced, mostly because I have yet to see a compelling use case for it - it sounds to me like too much work for basically no gain. However, I'm open to being convinced, especially if I were to be shown a valid or, even better, real-world use-case where this behaviour would come in handy. Or, if someone could present a way to implement static (or dynamic) dispatch in the shim, while keeping the shim "thin" and "simple". If adding this functionality adds enough complexity to the shim, there's also always the option to provide two distinct versions of the shim, one without this functionality, and one with this functionality, and the user would choose between them depending on their needs. But again, I worry that it's just a waste of time and nobody will use the more complex version. Hopefully that clarifies why I haven't worked on this yet. I am happy to accept contributions here, especially if you can find a way to assuage some of my worries above. |
I recently discovered that the module Touch VTT patched their shim to support inherited method wrapping. It seems that they wish to wrap This proves that I was wrong and there is indeed a use-case of inherited wrapping. Going back to the above discussion, I believe the only acceptable solution is basic static dispatch. The v1.1.2.0 shim now supports very basic static dispatch to handle instance/inherited wrapping:
I have also added this case to the shim test suite, and it is now passing. I also took the chance to do some small shim cleanup, to make it less fragile. This was all done as part of commit 76040fc. I will be closing this issue for now, as I consider it fixed. |
Since v1.0.4, the full libWrapper library officially supports instance-specific as well as inherited-method wrapping.
Unfortunately, adding this to the shim would be quite a bit of work, and as such the shim does not support this as of now.
For example, the following will work with the full library but throw an exception with the shim:
It is worth brainstorming whether this could be implemented in a simple way that would be appropriate for the shim.
The text was updated successfully, but these errors were encountered: