Since v1.0.4, the full libWrapper library officially supports instance-specific as wel

Shim does not support instance-specific / inherited-method wrapping. about fvtt-lib-wrapper HOT 3 CLOSED

ruipin commented on June 11, 2024

Shim does not support instance-specific / inherited-method wrapping.

from fvtt-lib-wrapper.

Comments (3)

Varriount commented on June 11, 2024

What makes implementing this complicated? (I'm thinking of taking a stab at this).

from fvtt-lib-wrapper.

ruipin commented on June 11, 2024

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

    
           test('Wrapper: Replace on instance', function(t) { 
        
           	// Replace on instance 
        
           	class A { 
        
           		x() { 
        
           			return 1; 
        
           		} 
        
           	} 
        
           	let originalValue = 1; 
        
           	let a = new A(); 
        
           	t.equal(a.x(), originalValue, 'Original'); 
        
           	wrap_front(A.prototype, 'x', function(original) { 
        
           		t.equal(original(), originalValue, 'xWrapper 1'); 
        
           		return 10; 
        
           	}); 
        
           	t.equal(a.x(), 10, "Wrapped with 10"); 
        
           	// Assign directly to a, not to A.prototype 
        
           	a.x = function() { return 20; }; 
        
           	originalValue = 20; 
        
           	t.equal(a.x(), 10, 'Instance assign #1'); 
        
           	// Calling another instance should return the old value 
        
           	let b = new A(); 
        
           	originalValue = 1; 
        
           	t.equal(b.x(), 10, 'Instance assign #2'); 
        
           	// Done 
        
           	t.end(); 
        
           }); 
        
           test('Wrapper: Inherited Class', function(t) { 
        
           	class B { 
        
           		x() { 
        
           			return 1; 
        
           		} 
        
           	} 
        
           	class A extends B { 
        
           	} 
        
           	class C extends B { 
        
           	} 
        
           	let originalValue = 1; 
        
           	let a = new A(); 
        
           	t.equal(a.x(), originalValue, 'Original'); 
        
           	wrap_front(a, 'x', function(original) { 
        
           		t.equal(original(), originalValue, 'xWrapper 1'); 
        
           		return 10; 
        
           	}); 
        
           	t.equal(a.x(), 10, "Wrapped with 10"); 
        
           	// Assign directly to a, not to A.prototype 
        
           	a.x = function() { return 20; }; 
        
           	originalValue = 20; 
        
           	t.equal(a.x(), 10, 'Instance assign #1'); 
        
           	// Calling another instance should return the old value 
        
           	let a2 = new A(); 
        
           	t.equal(a2.x(), 1, 'Instance assign #2'); 
        
           	// Overriding C's prototype will wrap 'undefined' 
        
           	let originalValue2 = 1; 
        
           	wrap_front(C.prototype, 'x', function(original) { 
        
           		t.equal(original(), originalValue2, 'xWrapper 2'); 
        
           		return 8; 
        
           	}); 
        
           	let c = new C(); 
        
           	t.equal(c.x(), 8, "Wrapped with 8"); 
        
           	// Overriding B's prototype will work 
        
           	wrap_front(B.prototype, 'x', function(original) { 
        
           		t.equal(original(), originalValue2, 'xWrapper 3'); 
        
           		return 5; 
        
           	}); 
        
           	originalValue = 5; 
        
           	t.equal(a2.x(), 5, "Wrapped with 5"); 
        
           	// Overriding A's prototype will use B's wrapper 
        
           	wrap_front(A.prototype, 'x', function(original) { 
        
           		t.equal(original(), originalValue, 'xWrapper 4'); 
        
           		return 7; 
        
           	}); 
        
           	t.equal(a2.x(), 7, "Wrapped with 7"); 
        
           	// Done 
        
           	t.end(); 
        
           });

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:

Instance method
Parent class method
Child class method

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:
c46e721#diff-fb0123441a9fd864139a9c0bb72007d887cbbe92c9c51f39751c4c6a734b5862

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:

Parent class method
Child class method
Instance method

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. Conclusion

I 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.

from fvtt-lib-wrapper.

ruipin commented on June 11, 2024

I recently discovered that the module Touch VTT patched their shim to support inherited method wrapping. It seems that they wish to wrap MeasuredTemplate.prototype._onClickLeft, which is inherited from PlaceableObject, without wrapping all PlaceableObjects. This means that shim modifications were required.

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:

On wrapping, the shim will search the instance/inheritance hierarchy for the desired method if it is otherwise not present in the supplied object, and will copy it to said object if found. Hopefully this will suffice for most cases.

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.

from fvtt-lib-wrapper.

Shim does not support instance-specific / inherited-method wrapping. about fvtt-lib-wrapper HOT 3 CLOSED

Comments (3)

1. Is it necessary?

2. Is it useful?

3. Is the implementation cost too big?

4. Conclusion

Related Issues (20)

Recommend Projects

React

Vue.js

Typescript

TensorFlow

Django

Laravel

D3

Recommend Topics

javascript

web

server

Machine learning

Visualization

Game

Recommend Org

Facebook

Microsoft

Google

Alibaba

D3

Tencent

	test('Wrapper: Replace on instance', function(t) {
	// Replace on instance
	class A {
	x() {
	return 1;
	}
	}


	let originalValue = 1;
	let a = new A();
	t.equal(a.x(), originalValue, 'Original');


	wrap_front(A.prototype, 'x', function(original) {
	t.equal(original(), originalValue, 'xWrapper 1');
	return 10;
	});
	t.equal(a.x(), 10, "Wrapped with 10");


	// Assign directly to a, not to A.prototype
	a.x = function() { return 20; };
	originalValue = 20;
	t.equal(a.x(), 10, 'Instance assign #1');


	// Calling another instance should return the old value
	let b = new A();
	originalValue = 1;
	t.equal(b.x(), 10, 'Instance assign #2');


	// Done
	t.end();
	});

	test('Wrapper: Inherited Class', function(t) {
	class B {
	x() {
	return 1;
	}
	}

	class A extends B {
	}

	class C extends B {
	}

	let originalValue = 1;
	let a = new A();
	t.equal(a.x(), originalValue, 'Original');

	wrap_front(a, 'x', function(original) {
	t.equal(original(), originalValue, 'xWrapper 1');
	return 10;
	});
	t.equal(a.x(), 10, "Wrapped with 10");


	// Assign directly to a, not to A.prototype
	a.x = function() { return 20; };
	originalValue = 20;
	t.equal(a.x(), 10, 'Instance assign #1');


	// Calling another instance should return the old value
	let a2 = new A();
	t.equal(a2.x(), 1, 'Instance assign #2');


	// Overriding C's prototype will wrap 'undefined'
	let originalValue2 = 1;
	wrap_front(C.prototype, 'x', function(original) {
	t.equal(original(), originalValue2, 'xWrapper 2');
	return 8;
	});
	let c = new C();
	t.equal(c.x(), 8, "Wrapped with 8");


	// Overriding B's prototype will work
	wrap_front(B.prototype, 'x', function(original) {
	t.equal(original(), originalValue2, 'xWrapper 3');
	return 5;
	});
	originalValue = 5;
	t.equal(a2.x(), 5, "Wrapped with 5");


	// Overriding A's prototype will use B's wrapper
	wrap_front(A.prototype, 'x', function(original) {
	t.equal(original(), originalValue, 'xWrapper 4');
	return 7;
	});
	t.equal(a2.x(), 7, "Wrapped with 7");


	// Done
	t.end();
	});