NativeScript / ios-jsc

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

Can't Extend Native Classes with ES6 #818

Open NickIliev opened 6 years ago

NickIliev commented 6 years ago

From @vbresults on November 6, 2017 11:22

I ran into this when setting a delegate in my project which is TypeScript compiled down to ES6. The problem would be that application.js expects a native delegate object while it's a JS function in ES6.

Copied from original issue: NativeScript/NativeScript#5038

ginev commented 6 years ago

@vbresults can we have a snippet that we can use to reproduce the case?

ghost commented 6 years ago
class Delegate extends UIResponder implements UIApplicationDelegate {
    public static ObjCProtocols = [UIApplicationDelegate];

    applicationDidReceiveRemoteNotification(application: UIApplication, userInfo: NSDictionary<string, any>) {
        // ...<snip>
    }

    // ...<snip>
}

const application = require("application");
application.ios.delegate = Delegate;
tdermendjiev commented 6 years ago

Hi @vbresults, are you getting any errors while building/running?

ginev commented 6 years ago

@vbresults - this looks like it should be working as expected. The application.ios property returns an instance of the IOSApplication class which we internally use to represent an iOS app. Its delegate property accepts a type, not an instance of it - so you basically need to set it to point to the class which instance you would like to use as a custom delegate.

This article might be helpful in resolving the case: https://docs.nativescript.org/core-concepts/application-lifecycle#ios-uiapplicationdelegate

I don't see you calling the start() method of the app. Can you share some more code here?

ghost commented 6 years ago

@tdermendjiev Yes, the error is that Delegate is not a native class but instead a JS function.

tdermendjiev commented 6 years ago

@vbresults it would be useful if you provide the exact error with the stack trace as well as the whole content of the js file.

ghost commented 6 years ago

@tdermendjiev It's triggered by this line in application.ios.js:

UIApplicationMain(0, null, null, iosApp && iosApp.delegate ? NSStringFromClass(iosApp.delegate) : NSStringFromClass(Responder));

NSStringFromClass expects a class but instead it's getting a JS function. My guess is that NS uses some sort of built-in __extends function which does not affect ES6+ native extends.

Make sure lib is set to es6 and dom in tsconfig.json.

tdermendjiev commented 6 years ago

@vbresults I managed to reproduce the issue and indeed its related to the extends function implemented by the ios runtime. ES6 extends has a different behavior and it doesn't call runtime's extends. That said, ES6 is not supported by both the iOS and Android runtimes. We don't plan supporting ES6 in the near future.

ghost commented 6 years ago

@tdermendjiev That's unfortunate. Native ES6 does not have some of the limitations that __extends does (specifically around inheritance and calling context); ES6 really helps to clean up my code.

ginev commented 6 years ago

@vbresults ES6 support will certainly become more demanded and we will have to support it at one point. By saying that we do not plan it for the near future our point is that we do not plan it for the upcoming two or three releases of NativeScript. If the demand for it increases, we will escalate its implementation.

louishfok commented 6 years ago

+1 es6+ is definitely where the industry is going. Any updates on this?

bradmartin commented 5 years ago

Ran into this tonight. With the recent updates to the android runtime to use the updated V8 versions. I've been running many apps with the tsconfig outputting es7 to use native async/await. Tonight I was working on updating an app that implements a delegate for the social-login plugin and the app crashed on start up and led me here. So with android supporting es7 I'd definitely think the iOS side should be moving in a forward direction.

Is the issue in the JSCore side? Curious how that works 😄

mbektchiev commented 5 years ago

I have both good and bad news regarding this issue. The good one is that I managed to come up with a workaround for now, and bad is that we still haven't planned to invest time in making it work automatically.

const Del = (UIResponder as any).extend({
    applicationDidBecomeActive(application: UIApplication): void {
        console.log("applicationDidBecomeActive", application);
    }
}, {
    protocols: [UIApplicationDelegate]
});

import * as application from 'application';

application.ios.delegate = Del;

application.run({ moduleName: 'app-root' });

This snippet casts the base class to any and calls the provided by NativeScript extend function. This way you can use ES2015 target for all other classes and at the same time circumvent the limitation that native classes cannot be subclassed. I hope this way you'll be able to switch to "target": "es2015".

One of the approaches for adding such support that I was thinking of was to provide a function attached to the global object, that has to be called manually for each class which extends a native class. For example something like this:

class Delegate extends UIResponder implements UIApplicationDelegate {
    public static ObjCProtocols = [UIApplicationDelegate];

    applicationDidReceiveRemoteNotification(application: UIApplication, userInfo: NSDictionary<string, any>) {
        // ...<snip>
    }

    // ...<snip>
}

__ns_extend_native(Delegate, UIResponder);

Maybe we can log a new feature request for that, if you think that this idea is good.

NathanWalker commented 5 years ago

@mbektchiev helpful idea - I just came across this today as well. I like your suggestion however perhaps instead of a rather obj-c'y kind of name like __ns_extend_native perhaps could be more inline with typical JS framework styles and consistent with {N} itself.

There's probably a whole bunch of convenient helpers to help {N} developers built on top of native apis therefore I'd suggest something like this:

import { nativeExtend } from 'tns-core-modules/utils/utils';

// class implementations...

nativeExtend(Delegate, UIResponder);

If there were a large enough amount of helpers just for native api targeted work I'd suggest a new barrel just for those specific helpers even.

Ideally the entire tns-core-modules namespace could be renamed to @nativescript and could have something like:

import { nativeExtend } from '@nativescript/utils';

// class implementations...

nativeExtend(Delegate, UIResponder);

/cc @bradmartin thoughts ^

bradmartin commented 5 years ago

I definitely lean toward the naming @NathanWalker mentions. I'd never know or think to look for __ns_extend_native on global. JS developers have heard for years to not pollute global. So having it be global, to some people, paints a bad image in their head. That may not be the best approach for developers of all skill levels. Having something in the core-modules, however, I'd expect.

Also the imports off of @nativescript has been something I've wanted and hoped for years. I've never liked import tns-core-modules for the reasons: its more troublesome to type by hand with the hyphens and you have to know what tns whereas nativescript is crystal clear what you are importing from. Similar to how other frameworks and companies publish packages.

Off topic question slightly, does webpack remove anything from global if not used?

mbektchiev commented 5 years ago

I agree with the concerns for global pollution, but currently by design every single C function, Objective-C class, protocol, etc. do pollute it as they are attached as its properties. As such a new function, will have to be implemented in Objective-C it will also have to be a property of global.

Having said that I really like the idea to provide a more JS-friendly wrapper in the modules for the new function and not force developers to write the whatever-cumbursome-underscored-function-name we come up with.

I think it would be a major breaking change to remove all of these properties from global and it is currently not being considered. Likewise, I think that renaming tns-core-modules will definitely be another breaking change that we should carefully think over before deciding whether it's worthy to do.

bradmartin commented 5 years ago

Ahhh good to know about the existing C functions being on global. If that is consistent then it makes sense to stay consistent. Having those methods exposed in core-modules should provide a few benefits: no need to create a declaration that defines all the global methods available, more JS friendly for development workflow. Of course, the exported methods from core-modules can just call the global method it's aligned with. I'm not sure how "testable" that is but I imagine it's possible.

As for the having tns-core-modules be exported under @nativescript, I haven't done any research but I'm curious if it's possible to have some alias for packages so that either would be valid going forward so it's not a breaking change but rather an enhancement, if that makes sense 😄 . It might not be possible to do without getting hacky, but I'm not sure on this area. @NathanWalker has definitely spent more time thinking about this.

NathanWalker commented 5 years ago

Makes sense @mbektchiev - I was under impression that __ns_extend_native would be just a JavaScript method that would extend as suggested in this comment you provided ((UIResponder as any).extend({ ...etc. which works great actually.

If the method is just JS makes sense of course as an exported method from {N} framework barrels and not on global scope. I like native classes part of global scope with {N} actually, never bothered me.

However perhaps I was missing something with your suggestion of __ns_extend_native - is there a reason that would need to be a native method as opposed to a JavaScript helper which would simply use .extend as you had suggested and which works great?

bigopon commented 5 years ago

I was extending Array and realized NS always wanted to target es5, then look up on GG to see if ES6 is supported, then led here. Would be nice if the team could provide some context to point out the limitation of the platform and how it connects to this incapability of supporting ES6. Atm, it feels much like a black box: "no you don't do it, that's all".

manijak commented 5 years ago

I have been going nuts for 3 days trying to figure out why the Protocol based class in my TypeScript plugin would not work. While the vanilla js version did work. And just by coincidence I stumbled upon this issue here.

After applying the workaround mentioned by @mbektchiev it finally worked! This should be posted on the documentation for subclassing objc classes. The example written there does not work: https://docs.nativescript.org/core-concepts/ios-runtime/how-to/ObjC-Subclassing#typescript-support

mbektchiev commented 5 years ago

Hi @manijak, thank you for bringing our attention to this inconsistency in the documentation and sorry for the inconvenience this has caused you. We'll update the article with a short notice about the problem and a link to this issue.

adrian-niculescu commented 5 years ago

Is ES6 going to actually be addressed? Using ES6 is no longer optional, but essential in 2019! The workaround to use TypeScript and compile it to ES5:

mbektchiev commented 5 years ago

It actually should be possible to switch to ES6 support now with the only drawback that you'll have to do this trick whenever you need to extend a native class in your TS code.

Other than that, we may be able to introduce the __ns_extend_native helper function which I suggested, but it still hasn't been planned for implementation by our team. If we see growing demand for this feature we will definitely raise its priority. But if the alternative method is working as expected, I don't see an immediate need for such a new feature in the runtime. Another solution could be to provide a helper function in the tns-core-modules, which would encapsulate the ugly cast to any.

adrian-niculescu commented 5 years ago

@mbektchiev Thank you for your response. This here is not a major issue and I can see why it's not a priority, but I know there are other problems that delay the support for ES6. It's a shame given that you created your premises - both JSCore and your V8 version support much newer features, so it's not the core runtime that is the blocker anymore.

For example, we are required to deliver ES6 code, and I can't simply use ES6 JavaScript modules inside NativeScript without having to transpile them with tsc, which complicates the build process and increases the development time.

NathanaelA commented 5 years ago

@mbektchiev - What about a feature request to actually update the extends keyword in JavaScript core engine to have the magic sauce that __ns_extend_native does. This way you depreciate the silly __ns_extend_native and the code starts magically working and this issue disappears as using extends is very common now.

farfromrefug commented 5 years ago

actually would love to see that one too. For me this important to have es2015 target in my tsconfig. right now.you need es5 to get __extends for native inheritance. but it means you loose native async/await and other features. what about an optional decorator? I think it should be doable.

Gamadril commented 5 years ago

It actually should be possible to switch to ES6 support now with the only drawback that you'll have to do this trick whenever you need to extend a native class in your TS code.

I work with JS Code only and need to extend NSObject for implementing own UITextFieldDelegate.

ES6 approach: class UITextFieldDelegateImpl extends NSObject does not work. VM is complaining about UITextFieldDelegateImpl not being a native object (Exception with thrown value: Error: This value is not a native object.)

How to apply the hack for TS transpiler above to pure JS code?

NathanaelA commented 5 years ago

@Gamadril - The link you quoted which links to the post; the very first example...

const Del = (UIResponder as any).extend({
    applicationDidBecomeActive(application: UIApplication): void {
        console.log("applicationDidBecomeActive", application);
    }
}, {
    protocols: [UIApplicationDelegate]
});

You have to currently use .extend() , you cannot write it as normal TS class extends NSObject

Gamadril commented 5 years ago

@NathanaelA: thanks, got it. TS notation confused me...

Got it working that way in ES6:

const UITextFieldDelegateImpl = NSObject.extend({
   ...
}, {
    protocols: [UITextFieldDelegate]
});

UITextFieldDelegateImpl.initWithOwner = function(owner) {
    const delegate = UITextFieldDelegateImpl.new();
    delegate._owner = owner;
    return delegate;
};
facetious commented 3 years ago

@NickIliev What is the status of this?

jitendraP-ashutec commented 3 years ago
var CompletionTarget = NSObject.extend({
                            "thisImage:hasBeenSavedInPhotoAlbumWithError:usingContextInfo:": function(
                                image,
                                error,
                                context
                            ) {
                                if (error) {
                                    console.error("Unable to save to library, please try again.")
                                }
                            }
                        }, {
                            exposedMethods: {
                                "thisImage:hasBeenSavedInPhotoAlbumWithError:usingContextInfo:": {
                                    returns: interop.types.void,
                                    params: [UIImage, NSError, interop.Pointer]
                                }
                            }
                        });

NSObject.extend({``` not working in native script 8. It gives the below error:

====== Assertion failed ====== Native stack trace: 1 0x109c5b9ff tns::Assert(bool, v8::Isolate) + 119 2 0x109c54617 tns::ClassBuilder::GetTypeEncodingType(v8::Isolate, v8::Local) + 121 3 0x109c52642 tns::ClassBuilder::ExposeDynamicMethods(v8::Local, objc_class, v8::Local, v8::Local, v8::Local) + 1332 4 0x109c520d3 tns::ClassBuilder::ExposeDynamicMembers(v8::Local, objc_class, v8::Local, v8::Local) + 219 5 0x109c516ee tns::ClassBuilder::ExtendCallback(v8::FunctionCallbackInfo const&) + 468 6 0x109d890a1 v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) + 609 7 0x109d8858a v8::internal::MaybeHandle v8::internal::(anonymous namespace)::HandleApiCallHelper(v8::internal::Isolate, v8::internal::Handle, v8::internal::Handle, v8::internal::Handle, v8::internal::Handle, v8::internal::BuiltinArguments) + 570 8 0x109d87bad v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate) + 253 9 0x10a65e7b9 Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_BuiltinExit + 57 JavaScript stack trace: