NativeScript / ios-jsc

NativeScript for iOS using JavaScriptCore
http://docs.nativescript.org/runtimes/ios
Apache License 2.0
298 stars 59 forks source link

Allow dynamic method overrides for subclasses of native ObjC classes #850

Open speigg opened 6 years ago

speigg commented 6 years ago

Currently, new javascript methods can't be added to a Obj-C subclass after the "extend" method is called. This makes it difficult (basically impossible) to override methods on various UIViewController and delegate classes used throughout the framework. For example, if I want to override preferredScreenEdgesDeferringSystemGestures on a Page's UIViewController (page.ios), I am unable to do so, as the UIViewController subclass (UIVIewControllerImpl, a private subclass of UIVIewController within the page module) is already established with it's native counterpart, and the native counterpart remains unaware of any new method overrides that are added dynamically.

However, if there were a way to update the native counterpart (global.__updateNativeClass?), then I could simply do the following:

const viewController = <UIVIewController>page.ios; // UIViewControllerImpl instance
const UIVIewControllerImpl = viewController.constructor; // UIVIewControllerImpl class

// Dynamically add override
UIVIewControllerImpl.prototype.preferredScreenEdgesDeferringSystemGestures = 
     () => UIRectEdge.All;

// update ObjC class with new js methods
global.__updateNativeClass(UIVIewControllerImpl); 

// Inform the view controller to check `preferredScreenEdgesDeferringSystemGestures`,
// which should call the overriden JS method we provided above
viewController.setNeedsUpdateOfScreenEdgesDeferringSystemGestures();

This should resolve the various issues (identified in https://github.com/NativeScript/NativeScript/issues/3264, https://github.com/NativeScript/NativeScript/issues/2471, and https://github.com/NativeScript/NativeScript/pull/2827) by allowing developers to easily fill in the gaps in native functionality which nativescript currently does not expose.

NathanaelA commented 6 years ago

@Speigg - This only works IF the delegate/controller is exported and accessible. In your case above you worked around their not publishing the viewcontroller; because the result value is exported (so you still have access to the original class via the .constructor.) However there are many cases where the delegate itself is created and used internally in class; and you have no access to get to it or where it is used. See: #2827 for the lengthy discussion.

speigg commented 6 years ago

@NathanaelA I'm not sure what you mean. Correct me if I am wrong, but I don't see a single case where you can't access a delegate/controller, and I don't think you gave any cases in https://github.com/NativeScript/NativeScript/pull/2827. More so, unless a delegate/controller instance is stored exclusively in module scope, it would be immediately garbage collected without any other references to it. So, by definition, all the delegates/controllers you care about are probably accessible (even if behind a “private” property), and therefore so are the delegate/controller constructors.

NathanaelA commented 6 years ago

WooHoo, @speigg -- you just made my day! You made me go and look at the current implementations.

UPDATE, TESTED this: The below idea DOES NOT work, a bug in the iOS NativeScript runtimes prevents the expect JavaScript method to work.

At the point in time I wrote that; they were not connected to any accessible properties. Many of them were assigned to local variables and were locked away AND the JavaScript engines were only ES5.

However, it does appear that almost all the delegates are now instantiated as this._delegate in the current version of TNS.

The NativeScript team may not have realized what they did; but this actually does make them very easily accessible in ES6 code. It opens up almost all(?) of them to very easy modification.

let originalPrototype = Object.getPrototypeOf(whateverClass._delegate);
/* OR */
let originalPrototype = whateverClass._delegate.constructor.prototype;

~~And once you have the delegate prototype you can do your prototype modifications to it as normal. And it is then part of the existing prototype chain making it 100% valid in the existing already instantiated delegates. You don't even have to create a new delegate... ;-)~~

I personally prefer adding the function to the prototype chain BEFORE it is instantiated (which is what I was arguing for in the 2827); but adding it AFTER is 99.9% of the time just as good. ;-)

Blog post detailing this now: http://fluentreports.com/blog/?p=558

speigg commented 6 years ago

@NathanaelA I don't think you can add any overrides to the prototype chain (that will be called by the native runtime) any time AFTER "extend" is called (even adding to the prototype chain before the first instance of the class is created won't work). That is what this issue seeks to correct.

NathanaelA commented 6 years ago

@speigg -- Actually, I'm pretty sure you can. I've done it before successfully with the application delegate in my ns-orientation plugin. That is how I know about the Object.getPrototypeOf function. :-) But for some reason I was still thinking I had no access to the other delegates, until you made me go look again. :-)

http://searchcode.nativescript.rocks/file/fedb6e76b8ed50d316784e2bfa8b4517e06f80eb/NativeScript-Orientation/src/orientation.js#264

speigg commented 6 years ago

@NathanaelA if that’s the case then this issue is a non-issue.... let me try some more tests

speigg commented 6 years ago

@NathanaelA Unfortunately, I don't think you are correct, as this doesn't seem to work:

if (frame.isIOS) {
    const UIViewControllerImpl = new page.Page().ios.constructor as typeof UIViewController;
    UIViewControllerImpl.prototype.preferredScreenEdgesDeferringSystemGestures = function() {
        console.log("I wish this worked!");  // doesn't work
        return UIRectEdge.All;
    }
}

Whereas this does work:

if (frame.isIOS) {
    const UIViewControllerImpl = new page.Page().ios.constructor as typeof UIViewController;

    const MyCustumUIViewController = UIViewController['extend'](Object.assign(
        {},
        UIViewControllerImpl.prototype,
        {
          // add instance method / property overrides here ...
          preferredScreenEdgesDeferringSystemGestures() {
            console.log("This function is called from native!"); // actually works
            return UIRectEdge.All;
          }
       }
    ));

    // workaround for not being to update the native class 
    const performNavigation = frame.Frame.prototype['performNavigation'];
    frame.Frame.prototype['performNavigation'] = function(navigationContext:{entry:frame.BackstackEntry}) {
        const page = navigationContext.entry.resolvedPage;
        const controller = (<typeof UIViewController>MyCustumUIViewController).new();
        controller['_owner'] = new WeakRef(page);
        controller.automaticallyAdjustsScrollViewInsets = false;
        controller.view.backgroundColor = new color.Color("white").ios;
        page['_ios'] = controller;
        page.setNativeView(controller.view);
        performNavigation.call(this, navigationContext);
    }
}

Though, if a global.__updateNativeClass feature were added, then the solution would be simple:

if (frame.isIOS) {
  const UIViewControllerImpl = new page.Page().ios.constructor as typeof UIViewController;
  UIVIewControllerImpl.prototype.preferredScreenEdgesDeferringSystemGestures = function() {
        console.log("Hypothetically, this text would be logged!"); // will work if feature in this issue exists
        return UIRectEdge.All;
    }
  global.__updateNativeClass(UIVIewControllerImpl); 
}
NathanaelA commented 6 years ago

@speigg - You appear to be correct; My original test in v8 engine worked 100% correctly; so I "assumed" it would work great the next time I needed to override a delegate. But NativeScript does appear to be doing something different (broken behavior based on the JS spec). Actually I think the issue is that Extend/ObjCProtocol must setup a unique internal cached object that is cloned from then on out (which does NOT match what normal JavaScript does, so unfortunately this is an area where NativeScript is technically breaking normal JavaScript behavior. )

Tests: First, update the ui/text-field file and export the real original delegate from say text-field. (i.e. what my original PR did) and then add some console logging to the delegate function you want to try overriding. (so you can see in the console if it is being called, or if the new version is being called).

Test 1. Require, and update the delegate prototype before it is ever used. This works perfectly; so we know we have everything correct. :grinning:

Test 2: Require, and create a new TextField; grab its delegate, then use getPrototypeOf (GPO) on delegate, update the prototype (you can either update the Original OR the GPO version.)

So it is as if the original is cached by NativeScript; as probably an optimization; but it does actually break normal JavaScript behavior. ;-(

I have a couple ideas left to try; but at this moment, the normal way to do it with JS won't work because NativeScript apparently has wrong behavior here. :-(

ginev commented 6 years ago

@speigg - this is indeed a limitation that we are aware of and will put on our TODO list for research and fixing.

NathanaelA commented 6 years ago

@speigg - To set your expectation properly; my PR was ~16 months ago; nothing has been done to fix the issue. (And I brought up the issue long before my PR, probably now going on over two years for this issue...)

As for your idea; it would allow easy modification of the delegate which certain people on the nativescript team really dislikes. So I don't see your idea being added, nor the bug we discovered preventing us from monkey patching the delegate after the fact, being fixed unless it has some other corner case that needs to be fixed.

speigg commented 6 years ago

I don't see your idea being added, nor the bug we discovered preventing us from monkey patching the delegate after the fact

@NathanaelA this issue (and the solution I proposed) has been specifically about that “bug” from the start. Also, it’s not that Nativescript has the wrong JavaScript behavior here, it’s just a feature in the bindings between JavaScript and Obj-C that they haven’t implemented. Anyways, it seems like @ginev has confirmed the limitation and expressed that they plan to correct it.

However, there is another solution. In theory, if the NativeScript team were to implement all relevant methods for the various delegate/controller subclasses (by including no-op implementations that just call the super methods), then everything would work. This solution does not require modifying the runtime, or directly exposing the Delegate/Controller classes in the NativeScript API... just filling in the various JavaScript subclasses of the native Obj-C classes with all of the potentially useful methods that are available. The downside is that such a solution would not as generalized, and would require updating the subclasses to make new APIs usable with each new iOS versions—but it would also be relatively simple to do.

NathanaelA commented 6 years ago

@speigg - I can point you to many places they have "confirmed" limits/issues and it isn't fixed (and may never be fixed). Just read the docs on the ios bridging area, or hell I have a PR I did in the 1.x days for a feature that was requested and they said they had a better way of doing it -- neither my PR nor the "better" fix ever occurred -- despite a revamp of that area several times. This unfortunately happens too frequently. :-(

I just didn't want you to get your hopes up it is something that is going to happen any time soon, my personal opinion, is it won't be ever fixed.

Honestly; I felt like I hit the lottery when I saw your original post and went back and looked at the delegate code, and realized there was a side channel to get access to it. And so I thought I finally had a solid way around a limitation that has hit me many times in the last couple years that I've been doing NativeScript development. It was very much like a second Christmas came, I was so ecstatic! :-) Unfortunately, I assumed perfectly normal javascript code would work, and even created a silly blog article on that assumption. :-)


Second, I would disagree with you on JS behavior. NativeScript should ALWAYS be running my JS code -- if it isn't running my JS code it is a bug in the implementation. So when I update the prototype; there is no reason it shouldn't be trampolining into the JavaScript system to run my JavaScript code and my NEW prototype. If you take Node, Edge, Chrome, Safari, or ANY and ALL other JS engine and do the following.

var x = function() { };  
x.prototype.print = function() { console.log("Hi is", this.hi); }
var y = new x();
y.print(); // prints "Hi is undefined"
var z = Object.getPrototypeOf(y);
z.hi = "hello";
y.print(); // Prints "Hi is hello";
var a = new x();
a.print() // Print "Hi is hello";

It works, and that is what the Spec states is how JavaScript is supposed to work. You do this even in NS and it works; however you do this with the NativeScript delegate (it might be all __extends native objects on iOS???) and it won't work. I think in pretty much in any JavaScript developers book that is buggy behavior. First, it does NOT match the expectations of how a JavaScript object/class should work, as they should work the same way ALWAYS. And it violates the JavaScript Spec sections 4.2.1 and 4.3.5 on how the prototype chain should work. If this object is a typeof x, then the current value of x.prototype.hi is what should be used.

Please note, despite working on NativeScript for several years, and writing many plugins; this is the first time I am aware that there is (imho) a nasty limitation (only on iOS) using extended Native platform objects. But this does explain why code in the past didn't work in some cases; and why I had to come up with other ways to deal with it. I've never seen this documented; and so it is another interesting gotcha.

NativeScript, I am assuming based on what happens with the delegate; if you use __extends on iOS native object it apparently does NOT trampoline back to your code properly; but to some other representation of the class that exists only in some weird form in memory. To be honest; this behavior actually concerns me greatly that their are several other corner cases I can think of now where NativeScript on iOS may be incorrect in its functionality since it is apparently is no longer tied back to the real original JavaScript object definition in some of its states. Which can lead to interesting bugs if you assume this JS object you have been handed works as any other JS object does.


Now as to your idea; I'm not sure how feasible that actually is. If you look at say the WebView; its delegate has well over 20 functions, I seem to recall. Some objects there are many delegate functions, others only a couple. Adding that many noop delegates functions/properties will increase the code bloat and the number of trampolines from native to javascript when each of them have to be run even if most people don't need access to most the delegates. We really don't want to cause iOS to slow down because it has to call dozens of noop functions because I "might" need access to one. Remember iOS is 100% interpreted, it does not have a JIT. Second, there are a couple delegate functions that if they exist; they cause a change in behavior; this is impossible to handle that scenario. If you add them because I might need them, then all apps are affected by it. If you don't add it; then I can't use it. But I love your brain storming! Thanks for your input, I certainly hope I'm wrong and we see something fix this; but after two years, a PR I did, and a couple really long discussions -- I tend to think it is a lost cause because of the views of the NativeScript team.

speigg commented 6 years ago

@ginev Any update on fixing this by chance? Fixing this one issue would make it possible (even easy) to get around all of the limitations in using various iOS APIs and functionality that are currently locked behind unchangeable delegates.

NathanaelA commented 6 years ago

@speigg - One small update; I did find out that at least the TabView does something a bit different in 4.x; I haven't spent any time researching any other delegates yet; but a client was needing some overrides on the TabView and I discovered this yesterday. :-)

It still basically; creates the unchangeable delegate in the constructor of the TabView; BUT it doesn't actually assign it to the native location until AFTER the loaded event is fired. So you can do the following:

onLoaded(args) {
    // This replaces the existing delegate with MY delegate
    args.object._delegate = MY_NEW_DELEGATE;
}

Now of course; you have to duplicate all the code in their delegate because they don't export it; but it is trivial to copy paste their entire delegate code into another JS file; make your changes to it and then replace it in the onloaded event.

I do not know if this is an oversight; or if they meant to do this. But if it is an oversight; then expect this cool bypass technique to disappear as they lock it down.