[TIMOB-24595] Windows: Hyperloop addEventListener doesn't work

GitHub Issue	n/a
Type	Bug
Priority	Critical
Status	Closed
Resolution	Fixed
Resolution Date	2017-05-01T18:22:54.000+0000
Affected Version/s	Release 6.1.0, Hyperloop 2.1.0
Fix Version/s	Release 6.1.0
Components	Windows
Labels	n/a
Reporter	Kota Iguchi
Assignee	Kota Iguchi
Created	2017-04-19T05:58:20.000+0000
Updated	2017-05-05T19:22:05.000+0000

Description

Use of Hyperloop addEventListener throws "unknow exception".

var win = Ti.UI.createWindow({ backgroundColor: 'green' }),
    Button = require('Windows.UI.Xaml.Controls.Button'),
    button = new Button();

button.Content = "PUSH";
button.addEventListener('Tapped', function (e) {
    alert('pushed');
});

win.add(button);
win.open();

*Expected* Pushing "PUSH" button should show "pushed" dialog. *Note* After some investigation, I found that at some point of Titanium SDK update, we updated uglify-js but it looks like it contains breaking changes. Uglify-js doesn't resolve constructor (init property of AST_Symbol on its [scope analyzer](http://lisperator.net/uglifyjs/scope). It looks like there was breaking changes there, maybe from uglify-js 2.8.x, because I saw uglify-js 2.7.x (Titanium SDK 6.1.0.v20170315131319) worked fine.

Comments

Kota Iguchi 2017-04-19

This is sample code to show the breaking-changes between uglify-js@2.7.0 and uglify-js@2.8.21

   var UglifyJS = require('uglify-js'),
       topLevel = UglifyJS.parse('var A = require("We.should.resolve.this"), a = new A(); a.addEventListener();');
   
   topLevel.figure_out_scope();
   
   var walker = new UglifyJS.TreeWalker(function(node) {
     if (node instanceof UglifyJS.AST_Call && node.expression.property == 'addEventListener') {
       console.log(node.expression.expression.thedef.init);
     }
   });
   
   topLevel.walk(walker);

*uglify-js@2.7.0 (Titanium SDK 6.1.0.v20170315131319)*

   AST_Node {
     end: 
      AST_Token {
        raw: undefined,
        file: null,
        comments_before: [],
        nlb: false,
        endpos: 54,
        endcol: 54,
        endline: 1,
        pos: 53,
        col: 53,
        line: 1,
        value: ')',
        type: 'punc' },
     start: 
      AST_Token {
        raw: undefined,
        file: null,
        comments_before: [],
        nlb: false,
        endpos: 50,
        endcol: 50,
        endline: 1,
        pos: 47,
        col: 47,
        line: 1,
        value: 'new',
        type: 'operator' },
     args: [],
     expression: 
      AST_Node {
        end: 
         AST_Token {
           raw: undefined,
           file: null,
           comments_before: [],
           nlb: false,
           endpos: 52,
           endcol: 52,
           endline: 1,
           pos: 51,
           col: 51,
           line: 1,
           value: 'A',
           type: 'name' },
        start: 
         AST_Token {
           raw: undefined,
           file: null,
           comments_before: [],
           nlb: false,
           endpos: 52,
           endcol: 52,
           endline: 1,
           pos: 51,
           col: 51,
           line: 1,
           value: 'A',
           type: 'name' },
        thedef: 
         { name: 'A',
           orig: [Object],
           scope: [Object],
           references: [Object],
           global: true,
           mangled_name: null,
           undeclared: false,
           constant: undefined,
           index: 0,
           id: 1,
           init: [Object] },
        name: 'A',
        scope: 
         AST_Node {
           end: [Object],
           start: [Object],
           body: [Object],
           cname: -1,
           enclosed: [Object],
           parent_scope: null,
           uses_eval: false,
           uses_with: false,
           functions: [Object],
           variables: [Object],
           directives: undefined,
           globals: [Object],
           nesting: 0 },
        frame: 0 } }

*uglify-js@2.8.21 (Titanium SDK 6.1.0.v20170417190415)*

   undefined

Hans Knöchel 2017-04-19

We should *not* use any of the Titanium API's in Hyperloop, I hope this was the only case. Instead of using addEventListener, removeEventListener etc, the developer expects to use the native event handling instead (delegates on iOS, callbacks on Android, etc.). Or do I understand this wrong?
Kota Iguchi 2017-04-19

{quote} > developer expects to use the native event handling instead {quote} We've been enabling addEventListener/removeEventListener on Hyperloop for Windows. (See also [Windows Runtime Direct API Access Event Handling](http://docs.appcelerator.com/platform/latest/#!/guide/Windows_Runtime_Direct_API_Access-section-src-43315893_WindowsRuntimeDirectAPIAccess-EventHandling)). But yes, this should be an interesting point to discuss. Basically on Windows, C\+\+/CX uses += to represent "add event listener" function like below. But the question is, do we have better style to represent this in JavaScript syntax? click_event_ = component->Tapped += ref new TappedEventHandler([this, ctx](Platform::Object^ sender, TappedRoutedEventArgs^ e) { // do something }); Currently we've been using addEventListener for this like below. component.addEventListener('Tapped', function(e) { // do something }); I was thinking addEventListener can be more familiar for JavaScript developers. We could do like below, regarding following C\+\+ syntax...but is this better? component.Tapped += function(e) { // do something }; Or...? component.Tapped += new TappedEventHandler(function(e) { // do something }); We could have better way to do this, any suggestions would be appreciated! (on)
Hans Knöchel 2017-04-19

Puuh, yeah the syntax is really hard to map to JavaScript. How we do we handle the same syntax for other class usages?
Kota Iguchi 2017-04-19

We just basically maps C\+\+/CX ref new with JavaScript new. Use of += for adding event listener is kind of special syntax in C\+\+/CX.
Hans Knöchel 2017-04-19

Yeah, the += could be misunderstood in JavaScript. I personally do like component.Tapped = new TappedEventHandler(function(e) { // do something }); but only if people will understand it and you would agree. It looks like the closest syntax without interfering using custom/non-native methods. You decide! :-)
Kota Iguchi 2017-04-21

I would prefer += because it clearly indicates we are "adding" something. (= actually means different behavior (assign) on C# & C\+\+). I've create TIMOB-24612, because this new syntax is not going to happen on release 2.1.0. component.Tapped += new TappedEventHandler(function(e) { // do something })
Kota Iguchi 2017-04-22

https://github.com/appcelerator/titanium_mobile_windows/pull/974
Hans Knöchel 2017-04-22

[~kota] Removing from "Hyperloop 2.1.0" milestone, since it's an SDK-PR.
Kota Iguchi 2017-05-02

Verified - https://github.com/appcelerator/titanium_mobile_windows/pull/974#pullrequestreview-35609940
Samir Mohammed 2017-05-02

Fix can be viewed in 6.2.0.v20170501192357 and 6.1.0.v20170501130025

JSON Source