Apache Cordova
  1. Apache Cordova
  2. CB-387

try/catch wrapper in native iOS code for cordova-js initialization firing alerts when page without cordova.js is loaded in

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.6.0
    • Fix Version/s: 1.6.0
    • Component/s: iOS
    • Labels:
      None
    • Environment:

      1.6.0rc1 <--- FYI!!

      Description

      Originally authored by code I wrote! My bad!

      See line 370 of CDVViewController.m:

      NSMutableString *result = [[NSMutableString alloc] initWithFormat:@"try{require('cordova/plugin/ios/device').setInfo(%@);}catch(e){alert('errorz1!!!');alert(JSON.stringify(e))}", [deviceProperties JSONString]];
      

      We should have a graceful way of degrading this, perhaps run a console.log instead? Suggestions?

        Activity

        Shazron Abdullah made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Shazron Abdullah added a comment -

        Thanks Patrick - good point about the better error messages, I'll add your suggestion.
        Didn't think about the chicken and egg problem re: console.log!

        In this case however, stringByEvaluatingJavascriptFromString can return a value from js and Objective-C can print it out, so that might solve the problem, at least for iOS. So instead of console.log, I would just return the error string - and on the Obj-C side if only something was returned, I print out the error and a hint.

        Show
        Shazron Abdullah added a comment - Thanks Patrick - good point about the better error messages, I'll add your suggestion. Didn't think about the chicken and egg problem re: console.log! In this case however, stringByEvaluatingJavascriptFromString can return a value from js and Objective-C can print it out, so that might solve the problem, at least for iOS. So instead of console.log, I would just return the error string - and on the Obj-C side if only something was returned, I print out the error and a hint.
        Hide
        Filip Maj added a comment -

        One more note! The feature to check the platform + version of the JS file against what the native platform returns is also logged in CB-385.

        Show
        Filip Maj added a comment - One more note! The feature to check the platform + version of the JS file against what the native platform returns is also logged in CB-385 .
        Hide
        Filip Maj added a comment -

        Also quick note that similar functionality was already committed to master in CB-364, where we added an alert function to the "utils" module.

        Show
        Filip Maj added a comment - Also quick note that similar functionality was already committed to master in CB-364 , where we added an alert function to the "utils" module.
        Hide
        Patrick Mueller added a comment -

        There's already a bug open on "improve the console object", and I think the things I've described in the comments above are a good fit for that bug.

        At this point, I think I'd just like to see more user-friendly messages (like hinting the user may not have included cordova-x.y.z.js in their .html file), in the current little wad of goo we have here. If something comes of the "improve the console" bug, I'll open a new set of bugs indicating how this wad of goo can further improved.

        Show
        Patrick Mueller added a comment - There's already a bug open on "improve the console object" , and I think the things I've described in the comments above are a good fit for that bug. At this point, I think I'd just like to see more user-friendly messages (like hinting the user may not have included cordova-x.y.z.js in their .html file), in the current little wad of goo we have here. If something comes of the "improve the console" bug, I'll open a new set of bugs indicating how this wad of goo can further improved.
        Hide
        Patrick Mueller added a comment - - edited

        After thinking about this for 5 more seconds, there's a chicken-and-egg problem with expecting a cordova global (got the case right this time) to be available, when it's not available

        Doesn't mean a consistent logging story used throughout Cordova isn't a bad thing, and I'll open a separate JIRA feature request on that.

        It also doesn't mean we can take advantage of that, somehow, in this case. For instance, it's possible we could design some kind of 'logger' module which could be used as a plain old module with require() (and/or attached to cordova) AND allow it to be literally included in some other eval()'y context like stringByEvaluatingJavaScriptFromString.

        Something to noodle over. I'm keen to provide AS MUCH diagnostic information as we can, and "fail fast" is probably a nice aspect to take into account, for fatal conditions like this. This can significantly cut down on the # of support requests we get for initial-error conditions.

        Show
        Patrick Mueller added a comment - - edited After thinking about this for 5 more seconds, there's a chicken-and-egg problem with expecting a cordova global (got the case right this time) to be available, when it's not available Doesn't mean a consistent logging story used throughout Cordova isn't a bad thing, and I'll open a separate JIRA feature request on that. It also doesn't mean we can take advantage of that, somehow, in this case. For instance, it's possible we could design some kind of 'logger' module which could be used as a plain old module with require() (and/or attached to cordova ) AND allow it to be literally included in some other eval() 'y context like stringByEvaluatingJavaScriptFromString . Something to noodle over. I'm keen to provide AS MUCH diagnostic information as we can, and "fail fast" is probably a nice aspect to take into account, for fatal conditions like this. This can significantly cut down on the # of support requests we get for initial-error conditions.
        Hide
        Patrick Mueller added a comment -

        Technically speaking, the message might more appropriately be "Error: executing module function 'setInfo' in module 'cordova/plugin/ios/device'". I think we should actually print that.

        Of course, we can infer a lot more about what's going on, based on the context. "cordova-js not available" captures the spirit of that contextual problem, but ... is not terribly helpful. Perhaps a hint "Have you included the iOS version of the cordova-x.y.z.js file?" Presumably the native code can plugin the version part of the string. This would actually be a good place to validate the version, and platform (can we sniff it?) and throw specific messages there.

        Lastly, if they don't have cordova loaded, then is console.log() even going to work? For iOS, what are our built-in, requires-no-natives options on notifying the user. alert() doesn't work here either, right? We could replace the body of their DOM with an error message, for instance.

        This is just one instance of being able to report problems back to users - we should actually have some functions available like log() and fatalError() (the names I typically now use for apps), that will log errors in a consistent place, and log an error then kill itself, respectively. The error we've caught here would be a candidate for calling fatalError().

        These functions should be rooted off Cordova. I'd suggest adding these TO Cordova, until Cordova gets too busy and then moving it to a logger property, or something.

        Show
        Patrick Mueller added a comment - Technically speaking, the message might more appropriately be "Error: executing module function 'setInfo' in module 'cordova/plugin/ios/device'". I think we should actually print that. Of course, we can infer a lot more about what's going on, based on the context. "cordova-js not available" captures the spirit of that contextual problem, but ... is not terribly helpful. Perhaps a hint "Have you included the iOS version of the cordova-x.y.z.js file?" Presumably the native code can plugin the version part of the string. This would actually be a good place to validate the version, and platform (can we sniff it?) and throw specific messages there. Lastly, if they don't have cordova loaded, then is console.log() even going to work? For iOS, what are our built-in, requires-no-natives options on notifying the user. alert() doesn't work here either, right? We could replace the body of their DOM with an error message, for instance. This is just one instance of being able to report problems back to users - we should actually have some functions available like log() and fatalError() (the names I typically now use for apps), that will log errors in a consistent place, and log an error then kill itself, respectively. The error we've caught here would be a candidate for calling fatalError() . These functions should be rooted off Cordova. I'd suggest adding these TO Cordova, until Cordova gets too busy and then moving it to a logger property, or something.
        Shazron Abdullah made changes -
        Field Original Value New Value
        Priority Major [ 3 ] Blocker [ 1 ]
        Hide
        Shazron Abdullah added a comment -

        I'm going to replace the format text with this below, unless there are better suggestions for the error message.

        try {
            require('cordova/plugin/ios/device').setInfo(%@);
        } catch (e) {
            console.log('Error: cordova-js not available.');
            console.log(JSON.stringify(e));
        }
        
        
        Show
        Shazron Abdullah added a comment - I'm going to replace the format text with this below, unless there are better suggestions for the error message. try { require('cordova/plugin/ios/device').setInfo(%@); } catch (e) { console.log('Error: cordova-js not available.'); console.log(JSON.stringify(e)); }
        Hide
        Filip Maj added a comment -

        Let's do it.

        Also a more meaningful message than my misspelled error would be good too

        Show
        Filip Maj added a comment - Let's do it. Also a more meaningful message than my misspelled error would be good too
        Hide
        Shazron Abdullah added a comment -

        I'm leaning towards console.log.

        Perhaps in the future, a modeless html dialog that can be dismissed could be used? (then we need to include this code in Cordova, short snippet for these kinds of scenarios)

        Show
        Shazron Abdullah added a comment - I'm leaning towards console.log. Perhaps in the future, a modeless html dialog that can be dismissed could be used? (then we need to include this code in Cordova, short snippet for these kinds of scenarios)
        Filip Maj created issue -

          People

          • Assignee:
            Shazron Abdullah
            Reporter:
            Filip Maj
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development