NativeScript / android

NativeScript for Android using v8
https://docs.nativescript.org/guide/android-marshalling
Apache License 2.0
525 stars 135 forks source link

Typescript target es6 does not generate native classes #1135

Closed farfromrefug closed 4 years ago

farfromrefug commented 6 years ago

I tried to switch my project to es6 with typescript. This works perfectly on iOS but on android it fails because of the native class generation.

I have a class like this in typescript:


@JavaProxy('org.myapp.BgServiceBinder')
export class BgServiceBinder extends android.os.Binder {
    service: WeakRef<BgService>;
    constructor() {
        super();
    }
    getService() {
        return this.service.get();
    }
    setService(service: BgService) {
        this.service = new WeakRef(service);
    }
}

If the typescript target is es5 the java class is generated correctly. But If I switch the target to es6 then the java class is not generated

darind commented 6 years ago

Currently the Android runtime does not support ES6 syntax. The reason this is failing is because the static binding generator does not recognize ES6 and fails to create the corresponding java classes.

ES6 is a work in progress and it will be introduced in future versions of the runtime, stay tuned.

farfromrefug commented 4 years ago

@darind i would like to reopen that issue. Yesterday i saw again that all my JS is es5 while v8 supports a lot more. Because of this we can't use es6 directly and thus we are missing on a lot of v8 optimizations.

What would it take for the runtime to support es6. I am thinking ios v8 too.

darind commented 4 years ago

Unfortunately the ES6 classes are not rich enough at the moment to cover all the native specific requirements such as implemented interfaces (in Android) and exposed native methods (not part of the base class prototype) and implemented protocols (in iOS). As a result some custom syntax will be required to be able to achieve this.

ES next decorators might someday be used but currently their support is completely missing from all JS engines.

As a consequence currently such feature doesn't make much sense to exist in both runtimes.

farfromrefug commented 4 years ago

@darind wait i am not sure i get your point. Could you explain a bit more here (for reference also) why es5 allows you those features when es6 does not? The thing i know would be tricky is the extend. But maybe in this case we could use webpack transform to transform only those classes as es5 classes.

darind commented 4 years ago

@farfromrefug, ES5 doesn't allow those features. It is {N} that has some custom syntax for them (on top of ES5):

let MyObject = java.lang.Object.extend("my.application.name.MyCustomObjectClassName", {
    interfaces: [ com.a.b.SomeInterface ],
    myMethod: function() {
    }
});

Notice the custom interfaces property.

And in iOS there's even more custom stuff:

var MyObject = NSObject.extend({
    myMethod: function (someParam) {
        console.log(someParam);
    }
}, {
    name: "MyCustomObjectClassName",
    protocols: [ SomeProtocol ],
    exposedMethods: {
        myMethod: { returns: interop.types.void, params: [ NSString ] }
    }
});

The thing i know would be tricky is the extend. But maybe in this case we could use webpack transform to transform only those classes as es5 classes.

Aren't you looking for extending native classes through ES6 (at least that's what you provided in your example)? Because if you are not extending native classes you can already do that in both runtimes (because both V8 and JSC already support ES6 classes):

class BaseClass {
    constructor(name) {
        this.name = name;
    }

    getName() {
        return this.name;
    }
}

class ChildClass extends BaseClass {
    constructor() {
        super("child class");
    }
}

let obj = new ChildClass();
console.log(obj.getName());
NathanaelA commented 4 years ago

@darind -- Their should be no reason that TS can't compile to ES6, correct, as long as anything native that is being extended is using the .extend call, correct?

darind commented 4 years ago

@NathanaelA, no ES6 is much more than classes. For example there are ES6 modules which our V8 based runtimes do not support. The reason for not supporting them is because the {N} modules are not ES6 modules syntax friendly yet. They still do have some hardcoded require/exports keywords in them and even if we retarget the typescript compiler to ES6, those keywords will remain and break the world. In order to fully support ES6 modules, everything that the V8 engine loads must be ES6 module compatible.

For example in Node.js, there's the .mjs file extension to indicate that the content is an ES6 module and in the browser there's the <script type="module"> syntax (and you cannot mix CommonJS modules with ES6 modules).

As you can see this is just one example of an ES6 feature that has its specifics when applied to {N}. We need to consider every feature separately. It is not as simple as retargeting the typescript compiler to ES6.

farfromrefug commented 4 years ago

@darind i get what you mean about es6 features not supported yet. But that s actually exactly where webpack/babel is good for. You can select exactly what you want as es6/es5...

Now lets look at es6 feature we could get because they are supported by v8 and even optimized in v8:

Now i know you dont support decorators explicitely but using class decorators (ts obviously) we can already do what you need to extend native class or declare interfaces Seeing that there is still and always will a compilation with webpack or other builder, we can always "transform" the source the way you want it.

I am sorry but honestly i dont see and blocking point. But i know it still needs to be thought of. And i even think it can already be achieved if i manage to do babel/webpack "transforms" for the 2 native class extending examples you posted.

darind commented 4 years ago

@farfromrefug, we already do babel/webpack. So what exactly is your question/feature request? What particular changes are you suggesting to the existing runtimes?

{N} already provides the necessary custom syntax for extending native classes and as long as you instruct your bundler to respect it you are good to go.

As far as the other ES6 features that you mentioned are concerned, we already support them so you can use them out of the box.

NathanaelA commented 4 years ago

@darind - Yeah, I have seen the wrapping code inside the v8 that wraps the code. Since we are using Wepack now is their any reason to have that code now in the engines... Webpack already wraps things. So could we eliminate that "extra" code inside the {N} v8 engines, and if for some reason we still needed it -- make that code be added by Webpack initially as part of the webpack process. Then a later step we could modify webpack to eliminate the wrapping if it sees it as a module.

This way their is an upgrade path to full ES6 modules.

darind commented 4 years ago

@NathanaelA, you are correct, the wrapping code is no longer necessary because of webpack. We are keeping it mainly for compatibility reasons. The runtimes unit tests are composed of multiple files and do not use a bundler. It makes it easier to read the code while developing the runtimes.

There are also some additional scripts that are not bundled and may still require this functionality - ts_helpers.js for Android and inspector_modules.js in iOS.

In terms of performance there's not much difference because we only have a single bundle at runtime and this wrapping happens only once. It used to be a more of a problem before webpack became required.

And finally, I am not very familiar with webpack, but as far as I know, it doesn't support outputting ES6 modules (contrary to Rollup).

So at the end of the day, I suppose webpack and ES6 modules are mutually exclusive. And since webpack is the de-facto standard in {N}, ES6 modules are not really on the table.

farfromrefug commented 4 years ago

@darind it was not a question in a sense. I wanted this issue to be re opened because i might not be the only one wanted this. And at the end this should be handled by {N}. I will try to make it work myself as a POC.

Now about webpack it seems your are right. I think i thought i did manage to do it because i was using es6 as target of tsconfig, which made it fail at runtime while trying to extend a native class. But the reason was not because it was keeping es6, but now i think maybe tsc was outputting es6 and then not using __extend.

Will try and run some test, because i also saw some things with svelte-native and mjs where i think i remember seeing class ... extends... in my vendor.js

EDIT: webpack 5 is in beta, we can already test it. The migration does not seem that hard

darind commented 4 years ago

@farfromrefug, that’s right. The reason this was failing when you switched to es6 is because when the typescript was transpiled it didn’t generate the expected __extend method but it kept the es6 extends class syntax.

It would be great if you can make this work as a POC. I would be happy to help with whatever I can. Don’t hesitate to ping me here or over slack.

farfromrefug commented 4 years ago

@darind I will try to make a POC. I will have to do it with Nativescript source though, because one breaking change is that every single plugin and @nativescript/core has to be compile with target es6 in tsc or it wont work.

Will try to find time for that

EDIT: maybe this issue should be moved to {N} main repo. I mean this is a global issue as the whole {N} workspace need to make the switch at the end. Runtime wise it is only a question of not packaging those unneeded js files anymore

darind commented 4 years ago

@farfromrefug, I think that you should start from the runtimes. If you change the target of @nativescript/core to es6, you will immediately hit a roadblock in the Android runtime which doesn't support ES6 modules (see my previous comment).

The correct approach to tackle this is to think of the JS syntax that you want the runtimes to support and try to implement it as a unit test in the runtime.

If on the other hand you don't want to change the existing support in the runtimes for __extends (and .extend()) then you may need to play with webpack to selectively opt-in for ES6 features.

farfromrefug commented 4 years ago

@darind i get your point. And yes i am thinking about the second solution for now. Seems like the easiest Actually this is what i intend to try: