Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make JavaProxy extends lazy #2031

Closed
wants to merge 1 commit into from
Closed

Conversation

jasssonpet
Copy link
Contributor

@jasssonpet jasssonpet commented Apr 25, 2016

Related to: #1563

ping @atanasovg, @KristinaKoeva

P.S. To view the diff without whitespaces: https://github.com/NativeScript/NativeScript/pull/2031/files?w=1

@jasssonpet jasssonpet self-assigned this Apr 25, 2016
@ns-bot ns-bot added cla: yes and removed cla: no labels Apr 25, 2016
@hshristov
Copy link
Contributor

@jasssonpet I though we won't sacrifice TypeScript in the process. When TS Class is wrapped in function you cannot instantiate in. I guess that is why you need
declare module com { ...

return global.__native(this);
}
lazyExtend(exports, () =>
__decorate([JavaProxy("com.tns.NativeScriptApplication")],
Copy link
Contributor

Choose a reason for hiding this comment

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

👎 can we find a way to not use __decorate here? I suggest we kill the JavaProxy decorator completely, and provide an API that adds a Java-JavaScript mapping. The result could probably look like:

javaClass("com.tns.NativeScriptActivity", () =>
    class NativeScriptActivity {
    //...
    }
);

This way the async thing needed by the snapshot will amount to a change in the javaClass code.

Oh, and I like the benefit of defining "Java" classes in closures to prevent access to those from the rest of the JS code.

@jasssonpet
Copy link
Contributor Author

jasssonpet commented Apr 26, 2016

@hshristov I understand it's tough to use TypeScript in such edge cases. According to the ES7 draft specification "A decorator is an expression", but TypeScript is emitting the following error: error TS1109: Expression expected.

I'm opening this PR to raise a discussion about the issue so that we can find the best solution for it.

@hshristov
Copy link
Contributor

If we are going to have such 'workarounds` I would vote for writing the whole class in Java and exposing some listener so that we don't need to extend/override it in JavaScript. In my opinion this is probably the best solution.

@jasssonpet jasssonpet force-pushed the jasssonpet/lazy-extend branch 5 times, most recently from b0499da to ea8665e Compare May 30, 2016 12:09
@jasssonpet
Copy link
Contributor Author

jasssonpet commented May 30, 2016

Hello there,

@PanayotCankov helped me to update this old pull request with your suggestions. Please review it one more time and say whether this is acceptable.

export function lazyExtend<T>(nativeName: string, classExtend: () => T): { value: T } {
let result = { value: null };
let register = () => result.value = global.__decorate([JavaProxy(nativeName)], classExtend());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sadly, this won't work because the static binding generator isn't able to make the link.

@jasssonpet
Copy link
Contributor Author

Updating the PR one more time.

@hshristov
Copy link
Contributor

-100
We are again falling back to JavaScript. You can't/shouldn't force people to write such code. If goes against our principle that TypeScript is first class citizen. I'm 100% we will forget about this hacks and will break the snapshot with some new check-in. A more robust approach needs to be implemented here.

@jasssonpet jasssonpet closed this Jun 1, 2016
@jasssonpet jasssonpet deleted the jasssonpet/lazy-extend branch June 24, 2016 10:48
@lock
Copy link

lock bot commented Aug 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Aug 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants