cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michal Mocny <mmo...@chromium.org>
Subject Re: Type checking in Cordova JS plugins
Date Wed, 21 Nov 2012 20:53:28 GMT
On Wed, Nov 21, 2012 at 3:51 PM, Andrew Grieve <agrieve@chromium.org> wrote:
> 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.
FWIW I hate API's that do this.  I feel it is always correct to teach
the user that they are making a mistake.

>
> 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 <b@brian.io> 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 <purplecabbage@gmail.com
>> >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 <agrieve@google.com> 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.
>> >
>>

Mime
View raw message