Skip to content

Conversation

petekanev
Copy link
Contributor

@petekanev petekanev commented Jan 16, 2018

The changes in the PR aim to allow the runtime to create and process class names for Android classes extended in TypeScript code in a fashion similar to how Android classes are extended in JavaScript - the resulting class names contain the Extendee (base class), the file name, line and column of the constructor function definition, as well as the name of the new class (if anonymous).

This is necessary so that Java classes are more easily debugged, and also allows TypeScript-extended Android classes to function properly in an uglified webpack build.

Addresses #898, #692, and a couple other issues across the NativeScript organization.

@petekanev petekanev added this to the 4.0.0 (TBD) milestone Jan 16, 2018
@petekanev petekanev self-assigned this Jan 16, 2018
@petekanev petekanev requested a review from Plamen5kov January 16, 2018 12:35
@NativeScript NativeScript deleted a comment from ns-bot Jan 16, 2018
@NativeScript NativeScript deleted a comment from ns-bot Jan 16, 2018
@petekanev
Copy link
Contributor Author

run ci

@ns-bot
Copy link

ns-bot commented Jan 16, 2018

💔

@Plamen5kov
Copy link
Contributor

Doesn't generate com.tns.NativeScriptActivity, and build fails on every template. The problem is within the js_parser because input for it is correct ..ui/frame/activity.js, but outputs bindings.txt is incorect (missing com.tns.NativeScriptActivity` information).

@petekanev
Copy link
Contributor Author

run ci

@ns-bot
Copy link

ns-bot commented Jan 17, 2018

💚

@petekanev
Copy link
Contributor Author

Note:

The AST JavaScript parser only recognizes constructors with the following _super. call

var _this = _super.call(this) || this;

Super calls normally emitted by TSC will look something like

return _super !== null && _super.apply(this, arguments) || this;

This discrepancy in formats can cause the ASBG's bindings.txt to contain entries whose lines and columns of occurrence are undefined.
view_1.ViewBase*_ftns_modules_tns_core_modules_ui_tab_view_tab_view_common_lundefined_cundefined__TabViewItemBase*_addChildFromBuilder,eachChild***
vs
android.support.v4.view.PagerAdapter*_ftns_modules_tns_core_modules_ui_tab_view_tab_view_l24_c32__PagerAdapterImpl*getCount,getPageTitle,instantiateItem,destroyItem,isViewFromObject,saveState,restoreState***

This is not a problem for the time being, so long as the specific format in the constructor functions is not changed.

var variableRHS = firstLineOfCtorFunction.declarations[0].init;
var variableRHSPaths = variableRHS._paths.filter(node => types.isCallExpression(node));
variableRHSPaths = variableRHSPaths.length > 0 ? variableRHSPaths[0] : null;
if (types.isMemberExpression(variableRHSPaths && variableRHSPaths.node.callee)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class X extends Y {
    constructor(a: () => number) {
        var x = a.call(null);
        super(x + 2);
    }
}

Generates:

var X = /** @class */ (function (_super) {
    __extends(X, _super);
    function X(a) {
        var _this = this;
        var x = a.call(null);
        _this = _super.call(this, x + 2) || this;
        return _this;
    }
    return X;
}(Y));

I am not sure that the var x = a.call(null); won't be considered as the super call in the current implementation. Perhaps we should add some check for the "_this" identifier on the left hand side of the assignment and probably the "_super" identifier on the left hand side of the member access.

@PanayotCankov
Copy link
Contributor

On the other hand, why don't we go one level up in the call stack and capture the location of the actual constructor function rather than the location of the super() call within?

@petekanev
Copy link
Contributor Author

@PanayotCankov we could get the location of the constructor function statically while traversing the JavaScript AST. However at runtime, when .extend() is called we have only enough information about the function name, line, and column of the call frame. We don't have any contextual information about the scope.

@ns-bot
Copy link

ns-bot commented Jan 23, 2018

💚

@ns-bot
Copy link

ns-bot commented Jan 25, 2018

💚

Copy link
Contributor

@Plamen5kov Plamen5kov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, we'll add tests once we figure out the best way to add the infrastructure

sis0k0 added a commit to NativeScript/nativescript-dev-webpack that referenced this pull request Jan 26, 2018
Depends on:
NativeScript/android#923
NativeScript/NativeScript#5257

BREAKING CHANGES

`uglifyMangleExcludes` is no longer exported from the
nativescript-dev-webpack plugin and shouldn't be used in the webpack
configuration.

Before:
```
// webpack.config.js
// ...
uglifyOptions: {
    mangle: { reserved: nsWebpack.uglifyMangleExcludes },
    // ...
}

```

After:
```
// webpack.config.js
// ...
uglifyOptions: {
    // ...
}

```
@petekanev petekanev merged commit 4e89392 into master Feb 2, 2018
@petekanev petekanev deleted the pete/ts-class-extend-names branch February 2, 2018 11:19
petekanev added a commit that referenced this pull request Feb 5, 2018
…#923)"

This reverts commit 4e89392.

The reasoning behind this is the file length limitation on Windows OS
petekanev added a commit that referenced this pull request Feb 5, 2018
Revert "fix(sbg): generate unique class names for ts-extended classes #923
sis0k0 added a commit to NativeScript/nativescript-dev-webpack that referenced this pull request Mar 12, 2018
Depends on:
NativeScript/android#923
NativeScript/NativeScript#5257

BREAKING CHANGES

`uglifyMangleExcludes` is no longer exported from the
nativescript-dev-webpack plugin and shouldn't be used in the webpack
configuration.

Before:
```
// webpack.config.js
// ...
uglifyOptions: {
    mangle: { reserved: nsWebpack.uglifyMangleExcludes },
    // ...
}

```

After:
```
// webpack.config.js
// ...
uglifyOptions: {
    // ...
}

```
sis0k0 added a commit to NativeScript/nativescript-dev-webpack that referenced this pull request Mar 12, 2018
Depends on:
NativeScript/android#923
NativeScript/NativeScript#5257

BREAKING CHANGES:

`uglifyMangleExcludes` is no longer exported from the
nativescript-dev-webpack plugin and shouldn't be used in the webpack
configuration.

Before:
```
// webpack.config.js
// ...
uglifyOptions: {
    mangle: { reserved: nsWebpack.uglifyMangleExcludes },
    // ...
}

```

After:
```
// webpack.config.js
// ...
uglifyOptions: {
    // ...
}

```
sis0k0 added a commit to NativeScript/nativescript-dev-webpack that referenced this pull request Mar 12, 2018
Depends on:
NativeScript/android#923
NativeScript/NativeScript#5257

BREAKING CHANGES:

`uglifyMangleExcludes` is no longer exported from the
nativescript-dev-webpack plugin and shouldn't be used in the webpack
configuration.

Before:
```
// webpack.config.js
// ...
uglifyOptions: {
    mangle: { reserved: nsWebpack.uglifyMangleExcludes },
    // ...
}

```

After:
```
// webpack.config.js
// ...
uglifyOptions: {
    // ...
}

```
@alexma01
Copy link

I still have the problem extending custom classes with javaproxy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants