Apache Cordova
  1. Apache Cordova
  2. CB-298

DEPRECATE "plugins" and "PhoneGap" global objects in JavaScript implementation

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Won't Fix
    • Affects Version/s: 1.4.0, 1.5.0, 1.6.0, 1.6.1, 1.7.0, 1.8.0
    • Fix Version/s: None
    • Component/s: CordovaJS

      Description

      There are two globals currently defined in Cordova-JS' common platform definition, which get dropped onto the `window` object: `plugins` and `PhoneGap`.

      Let's slate these for removal.

      In 1.6, let's set up deprecation notices for these.

        Activity

        Hide
        Randy McMillan added a comment -

        The man hours involved in patching all the plugins will be tremendous.

        Show
        Randy McMillan added a comment - The man hours involved in patching all the plugins will be tremendous.
        Hide
        Filip Maj added a comment -

        This is true, but we are going to break plugins at some point once we finalize / formalize the packaging format.

        Show
        Filip Maj added a comment - This is true, but we are going to break plugins at some point once we finalize / formalize the packaging format.
        Hide
        Patrick Mueller added a comment -

        I have no problem deprecating these, as long as we create some kind of document telling people how to provide "externs" for their plugin, and some guidance.

        For instance, BarcodeScanner exposes it's "business object" via window.plugins. What should it be doing instead?

        Also, what is the mechanism you're planning to use to deprecate window.plugins? I suppose we could check after running the plugin 'constructor' (specified via Cordova.addConstructor()) whether or not windows.plugin != null, and generate a console message.

        Show
        Patrick Mueller added a comment - I have no problem deprecating these, as long as we create some kind of document telling people how to provide "externs" for their plugin, and some guidance. For instance, BarcodeScanner exposes it's "business object" via window.plugins . What should it be doing instead? Also, what is the mechanism you're planning to use to deprecate window.plugins ? I suppose we could check after running the plugin 'constructor' (specified via Cordova.addConstructor() ) whether or not windows.plugin != null , and generate a console message.
        Hide
        Patrick Mueller added a comment -

        BTW, are the following 'API', or will they be deprecated, or what?

        Cordova.hasResource(aString)
        Cordova.addResource(aString)
        Cordova.addConstructor(aFunction)
        
        Show
        Patrick Mueller added a comment - BTW, are the following 'API', or will they be deprecated, or what? Cordova.hasResource(aString) Cordova.addResource(aString) Cordova.addConstructor(aFunction)
        Hide
        Filip Maj added a comment -

        addConstructor was added to the cordova module in cordova-js for backwards compatibility.

        hasResource and addResource do not exist in cordova-js.

        As far some guidance on how to move over, I would recommend employing cordova's define and require in cordova-js. For a hasResource equivalent, I would check if cordova.require('mymoduleid') throws an exception. If so, then go ahead and use cordova.define. If not, it's already defined, as the user does in your above example, simply return out. In the same vein, cordova.define can be used as a replacement for addResource.

        The only other change would be to the call to exec. Users can go with either calling cordova.exec directly, or if they want to use the same require syntax they can roll with cordova.require('cordova/exec').

        Show
        Filip Maj added a comment - addConstructor was added to the cordova module in cordova-js for backwards compatibility. hasResource and addResource do not exist in cordova-js. As far some guidance on how to move over, I would recommend employing cordova's define and require in cordova-js. For a hasResource equivalent, I would check if cordova.require('mymoduleid') throws an exception. If so, then go ahead and use cordova.define . If not, it's already defined, as the user does in your above example, simply return out. In the same vein, cordova.define can be used as a replacement for addResource . The only other change would be to the call to exec . Users can go with either calling cordova.exec directly, or if they want to use the same require syntax they can roll with cordova.require('cordova/exec') .
        Hide
        Filip Maj added a comment -

        Changed this issue to only set up deprecation notices for 1.6.0. A separate issue will be filed for removal in 2.0.

        Show
        Filip Maj added a comment - Changed this issue to only set up deprecation notices for 1.6.0. A separate issue will be filed for removal in 2.0.
        Hide
        Filip Maj added a comment -

        Fixed in f5d554.

        Show
        Filip Maj added a comment - Fixed in f5d554 .
        Hide
        Filip Maj added a comment -

        This commit was recently reverted.

        Show
        Filip Maj added a comment - This commit was recently reverted.
        Hide
        Filip Maj added a comment -

        Can we drop a console.log warning of deprecation / removal in 2.0 inside just the PhoneGap.exec method, Simon?

        Show
        Filip Maj added a comment - Can we drop a console.log warning of deprecation / removal in 2.0 inside just the PhoneGap.exec method, Simon?
        Hide
        Simon MacDonald added a comment -

        Hey Fil, do you have any suggestion on how to do that as PhoneGap.exec === cordova.exec now.

        Show
        Simon MacDonald added a comment - Hey Fil, do you have any suggestion on how to do that as PhoneGap.exec === cordova.exec now.
        Hide
        Filip Maj added a comment -

        After the PhoneGap object gets dropped down/created, can we override the PhoneGap.exec method by wrapping it in another function, similar to what the deprecation notice in the entire PhoneGap object did before, something like:

        var oldExec = PhoneGap.exec;
        PhoneGap.exec = function() {
          console.log('DEPRECATIONZ!!!!11one');
          oldExec.apply(PhoneGap, arguments);
        }
        

        That should work ya?

        Show
        Filip Maj added a comment - After the PhoneGap object gets dropped down/created, can we override the PhoneGap.exec method by wrapping it in another function, similar to what the deprecation notice in the entire PhoneGap object did before, something like: var oldExec = PhoneGap.exec; PhoneGap.exec = function() { console.log('DEPRECATIONZ!!!!11one'); oldExec.apply(PhoneGap, arguments); } That should work ya?
        Hide
        Bryce Curtis added a comment -

        We may want to consider the performance impact of console.log on every exec call - There are already deprecation notices for other calls all of which will/should be in docs.

        Show
        Bryce Curtis added a comment - We may want to consider the performance impact of console.log on every exec call - There are already deprecation notices for other calls all of which will/should be in docs.
        Hide
        Patrick Mueller added a comment -

        Indeed - for "marker" type of annotations, what I tend to do is only call the marker once. I have a centralized function in weinre that does this, actually, to prevent the same message from being printed endlessly. The function takes a 'key', which is fixed at the call site - in this one it might be "PhoneGap.exec", then the function which does the logging checks to see if that message has been logged yet.

        Show
        Patrick Mueller added a comment - Indeed - for "marker" type of annotations, what I tend to do is only call the marker once. I have a centralized function in weinre that does this, actually, to prevent the same message from being printed endlessly. The function takes a 'key', which is fixed at the call site - in this one it might be "PhoneGap.exec", then the function which does the logging checks to see if that message has been logged yet.
        Hide
        Filip Maj added a comment -

        I'd like to see the console.log's for deprecations in the cordova-js project as then we have one place to go to to drop deprecation notices.

        Without dropping into native frameworks for each implementation, is there a different way of enabling this?

        Show
        Filip Maj added a comment - I'd like to see the console.log's for deprecations in the cordova-js project as then we have one place to go to to drop deprecation notices. Without dropping into native frameworks for each implementation, is there a different way of enabling this?
        Hide
        Filip Maj added a comment -

        Hey all,

        I'd like to resolve this one. I think we all agree that deprecating these globals is in our best interest. So far the only thing brought up has been "performance impact" which I am not convinced is a big deal in this case (but please post bench results if you have data proving otherwise). Really, only plugins would be affected by this as the cordova APIs all route through the cordova object.

        @Pat, re:a single call to a console.log for a dep. warning, is that good enough? Isn't the point of a deprecation warning to be annoying and visible enough to a user so that (s)he makes recommended changes to their code.

        Show
        Filip Maj added a comment - Hey all, I'd like to resolve this one. I think we all agree that deprecating these globals is in our best interest. So far the only thing brought up has been "performance impact" which I am not convinced is a big deal in this case (but please post bench results if you have data proving otherwise). Really, only plugins would be affected by this as the cordova APIs all route through the cordova object. @Pat, re:a single call to a console.log for a dep. warning, is that good enough? Isn't the point of a deprecation warning to be annoying and visible enough to a user so that (s)he makes recommended changes to their code.
        Hide
        Simon MacDonald added a comment -

        Isn't the plan just to remove these in 2.0.0?

        Show
        Simon MacDonald added a comment - Isn't the plan just to remove these in 2.0.0?
        Hide
        Filip Maj added a comment -

        Yeh but not without deprecating first. That seemed to piss our users off the first time we did it in 1.5

        Show
        Filip Maj added a comment - Yeh but not without deprecating first. That seemed to piss our users off the first time we did it in 1.5
        Hide
        Patrick Mueller added a comment -

        It would be nice to print a single message out for each "deprecated" thing that was accessed, but only the first time the "deprecated" thing is called, lest folks deal with LOTS of duplicate messages.

        Still not clear to me what our story is for plugins though. I sense we want to have folks access plugins via require(). Perhaps the first thing we should do is make sure the standard plugins actually work that way. Eg, what does require("plugins/camera") (or whatever we decide to call the camera plugin's module) return? Are the Camera object and the navigator.camera object somehow smushed together?

        Show
        Patrick Mueller added a comment - It would be nice to print a single message out for each "deprecated" thing that was accessed, but only the first time the "deprecated" thing is called, lest folks deal with LOTS of duplicate messages. Still not clear to me what our story is for plugins though. I sense we want to have folks access plugins via require() . Perhaps the first thing we should do is make sure the standard plugins actually work that way. Eg, what does require("plugins/camera") (or whatever we decide to call the camera plugin's module) return? Are the Camera object and the navigator.camera object somehow smushed together?
        Hide
        Drew Walters added a comment -

        Even though this is sort of off topic for this issue, thought I'd mention something since it seems relevant to Patrick's question. In my mockup of a BlackBerry implementation where everything is a plugin I maintained Camera and navigator.camera as separate modules [1] which are defined by cordova.define and then added to the global namespace with cordova.addPlugin.

        External plugins could easily follow this model and even expose themselves on window.plugins again if they wanted to.

        [1] https://github.com/deedubbu/cordova-blackberry-pluggable/tree/master/ext/cordova.camera/js/common

        Show
        Drew Walters added a comment - Even though this is sort of off topic for this issue, thought I'd mention something since it seems relevant to Patrick's question. In my mockup of a BlackBerry implementation where everything is a plugin I maintained Camera and navigator.camera as separate modules [1] which are defined by cordova.define and then added to the global namespace with cordova.addPlugin. External plugins could easily follow this model and even expose themselves on window.plugins again if they wanted to. [1] https://github.com/deedubbu/cordova-blackberry-pluggable/tree/master/ext/cordova.camera/js/common
        Hide
        Filip Maj added a comment -

        Plugin story is being worked out in the PluginDesign wiki article and the recent "Pluginzation FTW" thread on the mailing list. Let's keep the discussion to ML and the PluginDesign draft.

        As for this issue, how about, as a happy medium, we implement deprecation notices for the PhoneGap and Cordova global object's methods, but route it through some mechanism that Pat suggests, wherein each deprecation notice only {{console.log}}s its message once. This should satisfy concerns about performance and also provide enough warning for our users.

        Yay/nay?

        Show
        Filip Maj added a comment - Plugin story is being worked out in the PluginDesign wiki article and the recent "Pluginzation FTW" thread on the mailing list. Let's keep the discussion to ML and the PluginDesign draft. As for this issue, how about, as a happy medium, we implement deprecation notices for the PhoneGap and Cordova global object's methods, but route it through some mechanism that Pat suggests, wherein each deprecation notice only {{console.log}}s its message once. This should satisfy concerns about performance and also provide enough warning for our users. Yay/nay?
        Hide
        Filip Maj added a comment -

        I'm gonna go ahead with:

        • adding deprecation notices for method calls off the PhoneGap and Cordova global objects
        • routing through a mechanism where only the first call to each such method logs a deprecation notice
        • annotating any relevant documentation with deprecation flags
        Show
        Filip Maj added a comment - I'm gonna go ahead with: adding deprecation notices for method calls off the PhoneGap and Cordova global objects routing through a mechanism where only the first call to each such method logs a deprecation notice annotating any relevant documentation with deprecation flags
        Hide
        Filip Maj added a comment -

        Patched in b80ff3.

        Show
        Filip Maj added a comment - Patched in b80ff3 .
        Hide
        Shazron Abdullah added a comment -

        Regression: PhoneGap.exec and Cordova.exec not defined.

        Found the problem, it's a timing issue:

        Before this function is called:
        https://github.com/apache/incubator-cordova-js/blob/master/lib/cordova.js#L281

        ... cordova.exec is not defined yet.

        Show
        Shazron Abdullah added a comment - Regression: PhoneGap.exec and Cordova.exec not defined. Found the problem, it's a timing issue: Before this function is called: https://github.com/apache/incubator-cordova-js/blob/master/lib/cordova.js#L281 ... cordova.exec is not defined yet.
        Hide
        Filip Maj added a comment - - edited

        Instead of clobbering on the window.PhoneGap and window.Cordova objects inside the cordova.js module, we can add those two to the common platform definition .js file and mimic the window.cordova object.

        Basically copy/paste lines 3 to 10 in common.js and change cordova to Cordova and PhoneGap.

        Shaz can you take care of this? I'm away at conference and don't have much time at the moment.

        Show
        Filip Maj added a comment - - edited Instead of clobbering on the window.PhoneGap and window.Cordova objects inside the cordova.js module , we can add those two to the common platform definition .js file and mimic the window.cordova object. Basically copy/paste lines 3 to 10 in common.js and change cordova to Cordova and PhoneGap . Shaz can you take care of this? I'm away at conference and don't have much time at the moment.
        Hide
        Shazron Abdullah added a comment -

        Thanks Fil, I'll take a look.

        Show
        Shazron Abdullah added a comment - Thanks Fil, I'll take a look.
        Hide
        Shazron Abdullah added a comment -

        Fil, took a look. This issue is not about how PhoneGap and Cordova is defined, but how to deprecate them. Your re-factor solution does not solve how to deprecate them, and that is the issue here.

        Show
        Shazron Abdullah added a comment - Fil, took a look. This issue is not about how PhoneGap and Cordova is defined, but how to deprecate them. Your re-factor solution does not solve how to deprecate them, and that is the issue here.
        Hide
        Filip Maj added a comment -

        Sorry, I'm confused. I thought we reverted the commit to add deprecation notices to all functions (except exec) because we found that exec was not defined on the old PhoneGap and Cordova globals?

        Although I don't think CB-560 has anything to do with the above two commits. That is, the last reversion should not fix CB-560.

        Show
        Filip Maj added a comment - Sorry, I'm confused. I thought we reverted the commit to add deprecation notices to all functions (except exec ) because we found that exec was not defined on the old PhoneGap and Cordova globals? Although I don't think CB-560 has anything to do with the above two commits. That is, the last reversion should not fix CB-560 .
        Hide
        Filip Maj added a comment - - edited

        Nevermind, I was wrong. Sorry Shaz.

        Just wrote up a test to prove what's going on. deprecateFunctions creates a completely new object. This causes exec to not exist on the PhoneGap and Cordova globals as they no longer reference the cordova object directly by reference.

        The test is here: https://github.com/filmaj/incubator-cordova-js/tree/apitests

        You can run the above test, and in cordova.js, swap between using deprecateFunctions and not, and see the failing tests in each case.

        Show
        Filip Maj added a comment - - edited Nevermind, I was wrong. Sorry Shaz. Just wrote up a test to prove what's going on. deprecateFunctions creates a completely new object. This causes exec to not exist on the PhoneGap and Cordova globals as they no longer reference the cordova object directly by reference. The test is here: https://github.com/filmaj/incubator-cordova-js/tree/apitests You can run the above test, and in cordova.js, swap between using deprecateFunctions and not, and see the failing tests in each case.
        Hide
        Filip Maj added a comment -

        I made one more commit to my apitests branch: https://github.com/filmaj/incubator-cordova-js/commit/f688762d861e56f1be60097e6d341826b1397df8

        This one passes the test and uses deprecateFunctions.

        One downside here is that the PhoneGap.exec and Cordova.exec functions will not have deprecation notices... still need a bit more to get that figured out.

        Show
        Filip Maj added a comment - I made one more commit to my apitests branch: https://github.com/filmaj/incubator-cordova-js/commit/f688762d861e56f1be60097e6d341826b1397df8 This one passes the test and uses deprecateFunctions . One downside here is that the PhoneGap.exec and Cordova.exec functions will not have deprecation notices... still need a bit more to get that figured out.
        Hide
        Shazron Abdullah added a comment -

        Did you see my comment:

        ---------------------
        Found the problem, it's a timing issue:
        Before this function is called:
        https://github.com/apache/incubator-cordova-js/blob/master/lib/cordova.js#L281

        ... cordova.exec is not defined yet.
        ---------------------

        I understand how deprecateFunctions works - in this case however, it can't "add" a deprecation notice to .exec because the function doesn't exist in the cordova object at this point.

        In any case, adding the PhoneGap and Cordova globals in common/common.js would suffice without adding the (if (!window.PhoneGap)) in lib/cordova.js since deprecateFunctions does not work (because of the reason I mentioned in the previous paragraph).

        Show
        Shazron Abdullah added a comment - Did you see my comment: --------------------- Found the problem, it's a timing issue: Before this function is called: https://github.com/apache/incubator-cordova-js/blob/master/lib/cordova.js#L281 ... cordova.exec is not defined yet. --------------------- I understand how deprecateFunctions works - in this case however, it can't "add" a deprecation notice to .exec because the function doesn't exist in the cordova object at this point. In any case, adding the PhoneGap and Cordova globals in common/common.js would suffice without adding the (if (!window.PhoneGap)) in lib/cordova.js since deprecateFunctions does not work (because of the reason I mentioned in the previous paragraph).
        Hide
        Shazron Abdullah added a comment -

        I'll add your code and re-test, we can leave this issue open until we figure out the deprecation notice thing.

        Show
        Shazron Abdullah added a comment - I'll add your code and re-test, we can leave this issue open until we figure out the deprecation notice thing.
        Hide
        Filip Maj added a comment -

        Punt to 1.9? sigh.

        Show
        Filip Maj added a comment - Punt to 1.9? sigh.
        Hide
        Filip Maj added a comment -

        I don't think we can do anything about this one. I will close as Won't Fix for now. We can reopen later if people have ideas on how to solve.

        Show
        Filip Maj added a comment - I don't think we can do anything about this one. I will close as Won't Fix for now. We can reopen later if people have ideas on how to solve.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development