Getting back to this - By coercing values, I mean if we're expecting a number, and they pass in the string "3", then we have our helper method change it to a number instead of throwing an exception. This would more closely match what browser APIs do, but is maybe not worth the extra complexity.
Okay, I'm going to go with throwing a TypeException, because I think it is useful to make a distinction between a failed operation, and passing invalid args. E.g. People's error callbacks will be easier to write if they don't need to check the error code to see if they just messed up the call. I'm going to get a start on this tomorrow, but will restrict the refactoring to just a few plugins at first so that I'm not changing too much before a release cut. On Fri, Nov 16, 2012 at 4:13 AM, Brian LeRoux <[email protected]> wrote: > 1. I like the proposal Andrew. We need type checking for sure and making it > optional for debugging even better. The light touch you have shown seems > good enough for now. I'd rather we did not augment natives...and not so > sure what you mean around coercing. > > 2. Success/error callbacks should be optional, says I. > > 3. When type errors happen, log & return, call errBack, or throw TypeError? > > I am very unsure about this one. Essentially this args checking is more for > our bridge than it is for user space so browser api symmetry less important > to me than doing right by the plugin author. It would seem this would > indicate an log and error callback. But again, not entirely certain that > will yield the most expected behavior. At least w/ throwing a TypeError we > fail noisily. =/ > > > > > On Fri, Nov 16, 2012 at 8:23 AM, Jesse MacFadyen <[email protected] > >wrote: > > > My answers/opinion > > > > 1. Up to the plugin developer > > 2. Up to the plugin developer > > 3. Up to the plugin developer > > > > However, that does not mean that we can't make the existing APIs > > handle params a little more consistently. > > > > Also, we could expose typeChecking helper methods, although they are > > pretty easy to come by. > > Array.isArray() could be polyfil'd or could be cordova.isArray() ... > > > > Cheers, > > Jesse > > > > Sent from my iPhone5 > > > > On 2012-11-15, at 12:16 PM, Andrew Grieve <[email protected]> wrote: > > > > > There's very little consistency when it comes to checking params in > > plugin > > > code. > > > > > > globalization.js< > > > https://git-wip-us.apache.org/repos/asf?p=incubator-cordova-js.git;a=blob;f=lib/common/plugin/globalization.js;h=a57062ce8b1e0cc95dde54f8ca600f6ee4876bfd;hb=HEAD > > >: > > > checks every args. logs errors and returns without calling errback, > does > > > not allow missing errCB. > > > > > > resolveLocalFileSystemURI.js< > > > https://git-wip-us.apache.org/repos/asf?p=incubator-cordova-js.git;a=blob;f=lib/common/plugin/resolveLocalFileSystemURI.js;h=c83b0aac3704c46782c3e19876afb180c32fc081;hb=HEAD > > >: > > > Allows missing successCB and errCB. Sanity checks URL has a colon in > it. > > > > > > notification.js< > > > https://git-wip-us.apache.org/repos/asf?p=incubator-cordova-js.git;a=blob;f=lib/common/plugin/notification.js;h=fbf9046a0e03bb6647219d7201b23f26f4d47084;hb=HEAD > > >: > > > doesn't check types at all > > > > > > contacts.js< > > > https://git-wip-us.apache.org/repos/asf?p=incubator-cordova-js.git;a=blob;f=lib/common/plugin/contacts.js;h=2f5a51a20c8ad14b96c4ef935a28686a9bd87eca;hb=HEAD > > >: > > > Throws TypeError if missing successCB, allows missing errCB > > > > > > compass.js< > > > https://git-wip-us.apache.org/repos/asf?p=incubator-cordova-js.git;a=blob;f=lib/common/plugin/compass.js;h=c97c1588bf69d2c94cde64736345a7d8b1dc32fe;hb=HEAD > > >: > > > Logs and returns if callbacks are of wrong type. success required, > error > > > optional. > > > > > > > > > So, the things to agree upon are: > > > > > > 1. Should we check types or not? > > > 2. Success / error callbacks : optional or not? > > > 3. When type errors happen, log & return, call errBack, or throw > > TypeError? > > > > > > > > > > > > For some extra inspiration, I played around with what it would look > like > > to > > > at least factor out type checking code. Have a look at the > > > module< > > > https://github.com/agrieve/incubator-cordova-js/blob/argscheck/lib/common/argscheck.js > > >and > > > it's > > > tests< > > > https://github.com/agrieve/incubator-cordova-js/blob/argscheck/test/test.argscheck.js > > >. > > > Also, here's< > > > https://github.com/agrieve/incubator-cordova-js/blob/argscheck/lib/common/plugin/globalization.js > > >what > > > it looks like being used in globalization.js. > > > > > > > > > Now, here's my thoughts for the three questions > > > 1. Should we check types or not? > > > - I think it is useful since incorrect args end up in exec() calls and > > > are therefore harder to debug when they are wrong. > > > - Perhaps instead of checking types, we add a helper for coercing > types? > > > This may only apply to strings / numbers though... > > > - I also kind of like the idea of being able to turn off type checking > > > for release mode. Having a common function to do the type checking > would > > > allow us to turn checking on/off for performance. > > > > > > 2. Success / error callbacks : optional or not? > > > - I'm not positive we need to make these all consistent > > > - If anything, I'd say we'd want them to be optional. > > > > > > 3. When type errors happen, log & return, call errBack, or throw > > TypeError? > > > - TypeError is my preference here. This matches what browser APIs do > > when > > > it doesn't like params. > > >
