You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by Andrew Grieve <ag...@google.com> on 2012/11/15 21:15:14 UTC
Type checking in Cordova JS plugins
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.
Re: Type checking in Cordova JS plugins
Posted by Andrew Grieve <ag...@chromium.org>.
On Thu, Nov 22, 2012 at 11:40 AM, Jesse MacFadyen
<pu...@gmail.com>wrote:
> In the original thread the rational was that things were inconsistent,
> but the examples were disparate.
>
> notification.alert(108);
> // This is valid and should not fail, and a callback function (win or
> lose) is not required.
>
> resolveLocalFileSystemURI.js
> // actually, it checks that there are not multiple colons, I wrote
> this to resolve a specific issue and fixed it in js to patch all
> platforms at once
>
> Backing up to the original questions ;
>
> 1. Should we check types or not?
> - sometimes yes, sometimes no, depends on the case, but this is
> difficult to achieve generically as a framework and should be left to
> the plugin developer.
> 2. Success / error callbacks : optional or not?
> - optional! Sometimes callback need to be optional, like console.log
> 3. When type errors happen, log & return, call errBack, or throw TypeError?
> - I think we should always log errors, and provide as much info as
> possible, however this can only really be done by leading by example.
> - generally, I think we should be calling the provided error callback
> if available, so that developers do not have to use both try/catch +
> callback.
>
The idea here is that type errors should not happen, so there's no need for
devs to add try/catches. e.g., you don't add a try/catch around calls to
browser APIs that guard against you passing in the wrong parameter types. I
think of this more like an assertion.
The example of the colons is one that my refactoring will not address. I'm
going to stick strictly to checking types so that there won't be much code.
>
> Andrew, your type checking code is certainly interesting, however I
> don't think it belongs in the cordova-js framework. Maybe this should
> be a standalone tool, in it's own repo ... are you still pushing to
> add this?
>
Yes. Still would like to add this. My motivation here is not to add
something new, but rather to delete lines of code within our core plugins.
I don't think it makes sense to have it in a separate repo, or else we'll
have cordova-js depending on this other repo.
>
> Cheers,
> Jesse
>
> Sent from my iPhone, forgive fat fingers.
>
>
> On 2012-11-22, at 6:55 AM, Andrew Grieve <ag...@chromium.org> wrote:
>
> > Good point Patrick. I'll go with a console.log + exception.
> >
> >
> > On Thu, Nov 22, 2012 at 9:01 AM, Patrick Mueller <pm...@gmail.com>
> wrote:
> >
> >> On Thu, Nov 22, 2012 at 6:00 AM, Brian LeRoux <b...@brian.io> wrote:
> >>
> >>> ya upon further consideration making these TypeException's feels right
> >>> since, ideally, this error would only be seen by a plugin author and
> not
> >>> something a plugin consumer (ideally)
> >>
> >> huh? I thought this was all about informing users when they pass
> invalid
> >> arguments?
> >>
> >> I'm usually in favor of "fail fast" - and so throwing an exception when
> you
> >> pass an invalid argument sounds right to me. The problem is that even
> >> though it's simple for us to fail fast by throwing an exception, we also
> >> need to make sure it's super obvious to the user that a failure has
> >> occurred. That's the hard part. Too many places where errors are
> silently
> >> consumed by the runtime.
> >>
> >> I think a console.log() would be appropriate - along with a thrown
> >> exception - lots of folks have access to a "console" these days. Or
> maybe
> >> we should come up with a new API - reportFailure() or something, which
> we
> >> could have - by default - just log to the console. For platforms that
> >> don't have an easily accessible console, they can override this to do
> >> something visible for their platform.
> >>
> >> --
> >> Patrick Mueller
> >> http://muellerware.org
> >>
>
Re: Type checking in Cordova JS plugins
Posted by Jesse MacFadyen <pu...@gmail.com>.
In the original thread the rational was that things were inconsistent,
but the examples were disparate.
notification.alert(108);
// This is valid and should not fail, and a callback function (win or
lose) is not required.
resolveLocalFileSystemURI.js
// actually, it checks that there are not multiple colons, I wrote
this to resolve a specific issue and fixed it in js to patch all
platforms at once
Backing up to the original questions ;
1. Should we check types or not?
- sometimes yes, sometimes no, depends on the case, but this is
difficult to achieve generically as a framework and should be left to
the plugin developer.
2. Success / error callbacks : optional or not?
- optional! Sometimes callback need to be optional, like console.log
3. When type errors happen, log & return, call errBack, or throw TypeError?
- I think we should always log errors, and provide as much info as
possible, however this can only really be done by leading by example.
- generally, I think we should be calling the provided error callback
if available, so that developers do not have to use both try/catch +
callback.
Andrew, your type checking code is certainly interesting, however I
don't think it belongs in the cordova-js framework. Maybe this should
be a standalone tool, in it's own repo ... are you still pushing to
add this?
Cheers,
Jesse
Sent from my iPhone, forgive fat fingers.
On 2012-11-22, at 6:55 AM, Andrew Grieve <ag...@chromium.org> wrote:
> Good point Patrick. I'll go with a console.log + exception.
>
>
> On Thu, Nov 22, 2012 at 9:01 AM, Patrick Mueller <pm...@gmail.com> wrote:
>
>> On Thu, Nov 22, 2012 at 6:00 AM, Brian LeRoux <b...@brian.io> wrote:
>>
>>> ya upon further consideration making these TypeException's feels right
>>> since, ideally, this error would only be seen by a plugin author and not
>>> something a plugin consumer (ideally)
>>
>> huh? I thought this was all about informing users when they pass invalid
>> arguments?
>>
>> I'm usually in favor of "fail fast" - and so throwing an exception when you
>> pass an invalid argument sounds right to me. The problem is that even
>> though it's simple for us to fail fast by throwing an exception, we also
>> need to make sure it's super obvious to the user that a failure has
>> occurred. That's the hard part. Too many places where errors are silently
>> consumed by the runtime.
>>
>> I think a console.log() would be appropriate - along with a thrown
>> exception - lots of folks have access to a "console" these days. Or maybe
>> we should come up with a new API - reportFailure() or something, which we
>> could have - by default - just log to the console. For platforms that
>> don't have an easily accessible console, they can override this to do
>> something visible for their platform.
>>
>> --
>> Patrick Mueller
>> http://muellerware.org
>>
Re: Type checking in Cordova JS plugins
Posted by Andrew Grieve <ag...@chromium.org>.
Good point Patrick. I'll go with a console.log + exception.
On Thu, Nov 22, 2012 at 9:01 AM, Patrick Mueller <pm...@gmail.com> wrote:
> On Thu, Nov 22, 2012 at 6:00 AM, Brian LeRoux <b...@brian.io> wrote:
>
> > ya upon further consideration making these TypeException's feels right
> > since, ideally, this error would only be seen by a plugin author and not
> > something a plugin consumer (ideally)
> >
>
> huh? I thought this was all about informing users when they pass invalid
> arguments?
>
> I'm usually in favor of "fail fast" - and so throwing an exception when you
> pass an invalid argument sounds right to me. The problem is that even
> though it's simple for us to fail fast by throwing an exception, we also
> need to make sure it's super obvious to the user that a failure has
> occurred. That's the hard part. Too many places where errors are silently
> consumed by the runtime.
>
> I think a console.log() would be appropriate - along with a thrown
> exception - lots of folks have access to a "console" these days. Or maybe
> we should come up with a new API - reportFailure() or something, which we
> could have - by default - just log to the console. For platforms that
> don't have an easily accessible console, they can override this to do
> something visible for their platform.
>
> --
> Patrick Mueller
> http://muellerware.org
>
Re: Type checking in Cordova JS plugins
Posted by Jesse <pu...@gmail.com>.
I appreciate that there is more to look at, I guess I would have just
preferred to see it in a feature branch.
I like it for the most part, the only issue I have is with this:
var argscheck = require('cordova/argscheck');
argscheck.checkArgs('fF', 'Device.getInfo', arguments);
I find this to be an increase in complexity ( granted it is small ),
at the expense of clarity.
I probably started a bigger conversation than I can sustain given that
I am on time off right now. I will be back at work on Monday and
happy to talk more then.
Cheers,
Jesse
On Thu, Nov 22, 2012 at 1:21 PM, Andrew Grieve <ag...@chromium.org> wrote:
> I don't mean to push ahead with this without your buy-in. What goes in can
> easily come out.
>
> I do want to give you concrete code to look at though, because I think our
> back-and-forth on this thread has made this change out to be more than it
> is.
>
>
> On Thu, Nov 22, 2012 at 4:02 PM, Jesse <pu...@gmail.com> wrote:
>
>> Nevermind then, guess it's in ...
>>
>>
>>
>>
>>
>> On Thu, Nov 22, 2012 at 12:51 PM, Andrew Grieve <ag...@chromium.org>
>> wrote:
>> > I just checked in argscheck.js and refactored all applicable lower-case
>> > plugin/*.js files to use it.
>> >
>> > It trims 4k off of cordova.ios.js and git tells me:
>> > 245 insertions(+), 323 deletions(-)
>> >
>> > I also found that a couple of our tests were passing invalid arg types :P
>> >
>> >
>> > On Thu, Nov 22, 2012 at 3:48 PM, Andrew Grieve <ag...@chromium.org>
>> wrote:
>> >
>> >> There's a good amount of code that currently check types on the public
>> >> API. My goal here is to shrink that code because it seems repetitive.
>> >>
>> >> Checking the types passed to exec may be useful in some cases too, but
>> we
>> >> don't currently do that. Probably, if we wanted to add this in, we'd
>> want
>> >> something more sophisticated that actually checked JSON schemas instead
>> of
>> >> just checking for typeof 'object'.
>> >>
>> >>
>> >>
>> >>
>> >> On Thu, Nov 22, 2012 at 1:19 PM, Brian LeRoux <b...@brian.io> wrote:
>> >>
>> >>> Ok, hold up, I'm missing something---I thought this was for the *bridge
>> >>> protocol validation* not the actual API surface end developers invoke.
>> >>>
>> >>> Ideally the public API would define their own exceptional paths. (Har
>> >>> har.)
>> >>> Example: geolocation api has error callbacks whereas File API has
>> >>> FileError
>> >>> (or some such / on bad connection and just blasting this w/o checking).
>> >>>
>> >>>
>> >>> On Thu, Nov 22, 2012 at 2:01 PM, Patrick Mueller <pm...@gmail.com>
>> >>> wrote:
>> >>>
>> >>> > On Thu, Nov 22, 2012 at 6:00 AM, Brian LeRoux <b...@brian.io> wrote:
>> >>> >
>> >>> > > ya upon further consideration making these TypeException's feels
>> right
>> >>> > > since, ideally, this error would only be seen by a plugin author
>> and
>> >>> not
>> >>> > > something a plugin consumer (ideally)
>> >>> > >
>> >>> >
>> >>> > huh? I thought this was all about informing users when they pass
>> >>> invalid
>> >>> > arguments?
>> >>> >
>> >>> > I'm usually in favor of "fail fast" - and so throwing an exception
>> when
>> >>> you
>> >>> > pass an invalid argument sounds right to me. The problem is that
>> even
>> >>> > though it's simple for us to fail fast by throwing an exception, we
>> also
>> >>> > need to make sure it's super obvious to the user that a failure has
>> >>> > occurred. That's the hard part. Too many places where errors are
>> >>> silently
>> >>> > consumed by the runtime.
>> >>> >
>> >>> > I think a console.log() would be appropriate - along with a thrown
>> >>> > exception - lots of folks have access to a "console" these days. Or
>> >>> maybe
>> >>> > we should come up with a new API - reportFailure() or something,
>> which
>> >>> we
>> >>> > could have - by default - just log to the console. For platforms
>> that
>> >>> > don't have an easily accessible console, they can override this to do
>> >>> > something visible for their platform.
>> >>> >
>> >>> > --
>> >>> > Patrick Mueller
>> >>> > http://muellerware.org
>> >>> >
>> >>>
>> >>
>> >>
>>
>>
>>
>> --
>> @purplecabbage
>> risingj.com
>>
--
@purplecabbage
risingj.com
Re: Type checking in Cordova JS plugins
Posted by Andrew Grieve <ag...@chromium.org>.
I don't mean to push ahead with this without your buy-in. What goes in can
easily come out.
I do want to give you concrete code to look at though, because I think our
back-and-forth on this thread has made this change out to be more than it
is.
On Thu, Nov 22, 2012 at 4:02 PM, Jesse <pu...@gmail.com> wrote:
> Nevermind then, guess it's in ...
>
>
>
>
>
> On Thu, Nov 22, 2012 at 12:51 PM, Andrew Grieve <ag...@chromium.org>
> wrote:
> > I just checked in argscheck.js and refactored all applicable lower-case
> > plugin/*.js files to use it.
> >
> > It trims 4k off of cordova.ios.js and git tells me:
> > 245 insertions(+), 323 deletions(-)
> >
> > I also found that a couple of our tests were passing invalid arg types :P
> >
> >
> > On Thu, Nov 22, 2012 at 3:48 PM, Andrew Grieve <ag...@chromium.org>
> wrote:
> >
> >> There's a good amount of code that currently check types on the public
> >> API. My goal here is to shrink that code because it seems repetitive.
> >>
> >> Checking the types passed to exec may be useful in some cases too, but
> we
> >> don't currently do that. Probably, if we wanted to add this in, we'd
> want
> >> something more sophisticated that actually checked JSON schemas instead
> of
> >> just checking for typeof 'object'.
> >>
> >>
> >>
> >>
> >> On Thu, Nov 22, 2012 at 1:19 PM, Brian LeRoux <b...@brian.io> wrote:
> >>
> >>> Ok, hold up, I'm missing something---I thought this was for the *bridge
> >>> protocol validation* not the actual API surface end developers invoke.
> >>>
> >>> Ideally the public API would define their own exceptional paths. (Har
> >>> har.)
> >>> Example: geolocation api has error callbacks whereas File API has
> >>> FileError
> >>> (or some such / on bad connection and just blasting this w/o checking).
> >>>
> >>>
> >>> On Thu, Nov 22, 2012 at 2:01 PM, Patrick Mueller <pm...@gmail.com>
> >>> wrote:
> >>>
> >>> > On Thu, Nov 22, 2012 at 6:00 AM, Brian LeRoux <b...@brian.io> wrote:
> >>> >
> >>> > > ya upon further consideration making these TypeException's feels
> right
> >>> > > since, ideally, this error would only be seen by a plugin author
> and
> >>> not
> >>> > > something a plugin consumer (ideally)
> >>> > >
> >>> >
> >>> > huh? I thought this was all about informing users when they pass
> >>> invalid
> >>> > arguments?
> >>> >
> >>> > I'm usually in favor of "fail fast" - and so throwing an exception
> when
> >>> you
> >>> > pass an invalid argument sounds right to me. The problem is that
> even
> >>> > though it's simple for us to fail fast by throwing an exception, we
> also
> >>> > need to make sure it's super obvious to the user that a failure has
> >>> > occurred. That's the hard part. Too many places where errors are
> >>> silently
> >>> > consumed by the runtime.
> >>> >
> >>> > I think a console.log() would be appropriate - along with a thrown
> >>> > exception - lots of folks have access to a "console" these days. Or
> >>> maybe
> >>> > we should come up with a new API - reportFailure() or something,
> which
> >>> we
> >>> > could have - by default - just log to the console. For platforms
> that
> >>> > don't have an easily accessible console, they can override this to do
> >>> > something visible for their platform.
> >>> >
> >>> > --
> >>> > Patrick Mueller
> >>> > http://muellerware.org
> >>> >
> >>>
> >>
> >>
>
>
>
> --
> @purplecabbage
> risingj.com
>
Re: Type checking in Cordova JS plugins
Posted by Jesse <pu...@gmail.com>.
Nevermind then, guess it's in ...
On Thu, Nov 22, 2012 at 12:51 PM, Andrew Grieve <ag...@chromium.org> wrote:
> I just checked in argscheck.js and refactored all applicable lower-case
> plugin/*.js files to use it.
>
> It trims 4k off of cordova.ios.js and git tells me:
> 245 insertions(+), 323 deletions(-)
>
> I also found that a couple of our tests were passing invalid arg types :P
>
>
> On Thu, Nov 22, 2012 at 3:48 PM, Andrew Grieve <ag...@chromium.org> wrote:
>
>> There's a good amount of code that currently check types on the public
>> API. My goal here is to shrink that code because it seems repetitive.
>>
>> Checking the types passed to exec may be useful in some cases too, but we
>> don't currently do that. Probably, if we wanted to add this in, we'd want
>> something more sophisticated that actually checked JSON schemas instead of
>> just checking for typeof 'object'.
>>
>>
>>
>>
>> On Thu, Nov 22, 2012 at 1:19 PM, Brian LeRoux <b...@brian.io> wrote:
>>
>>> Ok, hold up, I'm missing something---I thought this was for the *bridge
>>> protocol validation* not the actual API surface end developers invoke.
>>>
>>> Ideally the public API would define their own exceptional paths. (Har
>>> har.)
>>> Example: geolocation api has error callbacks whereas File API has
>>> FileError
>>> (or some such / on bad connection and just blasting this w/o checking).
>>>
>>>
>>> On Thu, Nov 22, 2012 at 2:01 PM, Patrick Mueller <pm...@gmail.com>
>>> wrote:
>>>
>>> > On Thu, Nov 22, 2012 at 6:00 AM, Brian LeRoux <b...@brian.io> wrote:
>>> >
>>> > > ya upon further consideration making these TypeException's feels right
>>> > > since, ideally, this error would only be seen by a plugin author and
>>> not
>>> > > something a plugin consumer (ideally)
>>> > >
>>> >
>>> > huh? I thought this was all about informing users when they pass
>>> invalid
>>> > arguments?
>>> >
>>> > I'm usually in favor of "fail fast" - and so throwing an exception when
>>> you
>>> > pass an invalid argument sounds right to me. The problem is that even
>>> > though it's simple for us to fail fast by throwing an exception, we also
>>> > need to make sure it's super obvious to the user that a failure has
>>> > occurred. That's the hard part. Too many places where errors are
>>> silently
>>> > consumed by the runtime.
>>> >
>>> > I think a console.log() would be appropriate - along with a thrown
>>> > exception - lots of folks have access to a "console" these days. Or
>>> maybe
>>> > we should come up with a new API - reportFailure() or something, which
>>> we
>>> > could have - by default - just log to the console. For platforms that
>>> > don't have an easily accessible console, they can override this to do
>>> > something visible for their platform.
>>> >
>>> > --
>>> > Patrick Mueller
>>> > http://muellerware.org
>>> >
>>>
>>
>>
--
@purplecabbage
risingj.com
Re: Type checking in Cordova JS plugins
Posted by Andrew Grieve <ag...@chromium.org>.
I just checked in argscheck.js and refactored all applicable lower-case
plugin/*.js files to use it.
It trims 4k off of cordova.ios.js and git tells me:
245 insertions(+), 323 deletions(-)
I also found that a couple of our tests were passing invalid arg types :P
On Thu, Nov 22, 2012 at 3:48 PM, Andrew Grieve <ag...@chromium.org> wrote:
> There's a good amount of code that currently check types on the public
> API. My goal here is to shrink that code because it seems repetitive.
>
> Checking the types passed to exec may be useful in some cases too, but we
> don't currently do that. Probably, if we wanted to add this in, we'd want
> something more sophisticated that actually checked JSON schemas instead of
> just checking for typeof 'object'.
>
>
>
>
> On Thu, Nov 22, 2012 at 1:19 PM, Brian LeRoux <b...@brian.io> wrote:
>
>> Ok, hold up, I'm missing something---I thought this was for the *bridge
>> protocol validation* not the actual API surface end developers invoke.
>>
>> Ideally the public API would define their own exceptional paths. (Har
>> har.)
>> Example: geolocation api has error callbacks whereas File API has
>> FileError
>> (or some such / on bad connection and just blasting this w/o checking).
>>
>>
>> On Thu, Nov 22, 2012 at 2:01 PM, Patrick Mueller <pm...@gmail.com>
>> wrote:
>>
>> > On Thu, Nov 22, 2012 at 6:00 AM, Brian LeRoux <b...@brian.io> wrote:
>> >
>> > > ya upon further consideration making these TypeException's feels right
>> > > since, ideally, this error would only be seen by a plugin author and
>> not
>> > > something a plugin consumer (ideally)
>> > >
>> >
>> > huh? I thought this was all about informing users when they pass
>> invalid
>> > arguments?
>> >
>> > I'm usually in favor of "fail fast" - and so throwing an exception when
>> you
>> > pass an invalid argument sounds right to me. The problem is that even
>> > though it's simple for us to fail fast by throwing an exception, we also
>> > need to make sure it's super obvious to the user that a failure has
>> > occurred. That's the hard part. Too many places where errors are
>> silently
>> > consumed by the runtime.
>> >
>> > I think a console.log() would be appropriate - along with a thrown
>> > exception - lots of folks have access to a "console" these days. Or
>> maybe
>> > we should come up with a new API - reportFailure() or something, which
>> we
>> > could have - by default - just log to the console. For platforms that
>> > don't have an easily accessible console, they can override this to do
>> > something visible for their platform.
>> >
>> > --
>> > Patrick Mueller
>> > http://muellerware.org
>> >
>>
>
>
Re: Type checking in Cordova JS plugins
Posted by Andrew Grieve <ag...@chromium.org>.
There's a good amount of code that currently check types on the public API.
My goal here is to shrink that code because it seems repetitive.
Checking the types passed to exec may be useful in some cases too, but we
don't currently do that. Probably, if we wanted to add this in, we'd want
something more sophisticated that actually checked JSON schemas instead of
just checking for typeof 'object'.
On Thu, Nov 22, 2012 at 1:19 PM, Brian LeRoux <b...@brian.io> wrote:
> Ok, hold up, I'm missing something---I thought this was for the *bridge
> protocol validation* not the actual API surface end developers invoke.
>
> Ideally the public API would define their own exceptional paths. (Har har.)
> Example: geolocation api has error callbacks whereas File API has FileError
> (or some such / on bad connection and just blasting this w/o checking).
>
>
> On Thu, Nov 22, 2012 at 2:01 PM, Patrick Mueller <pm...@gmail.com>
> wrote:
>
> > On Thu, Nov 22, 2012 at 6:00 AM, Brian LeRoux <b...@brian.io> wrote:
> >
> > > ya upon further consideration making these TypeException's feels right
> > > since, ideally, this error would only be seen by a plugin author and
> not
> > > something a plugin consumer (ideally)
> > >
> >
> > huh? I thought this was all about informing users when they pass invalid
> > arguments?
> >
> > I'm usually in favor of "fail fast" - and so throwing an exception when
> you
> > pass an invalid argument sounds right to me. The problem is that even
> > though it's simple for us to fail fast by throwing an exception, we also
> > need to make sure it's super obvious to the user that a failure has
> > occurred. That's the hard part. Too many places where errors are
> silently
> > consumed by the runtime.
> >
> > I think a console.log() would be appropriate - along with a thrown
> > exception - lots of folks have access to a "console" these days. Or
> maybe
> > we should come up with a new API - reportFailure() or something, which we
> > could have - by default - just log to the console. For platforms that
> > don't have an easily accessible console, they can override this to do
> > something visible for their platform.
> >
> > --
> > Patrick Mueller
> > http://muellerware.org
> >
>
Re: Type checking in Cordova JS plugins
Posted by Brian LeRoux <b...@brian.io>.
Ok, hold up, I'm missing something---I thought this was for the *bridge
protocol validation* not the actual API surface end developers invoke.
Ideally the public API would define their own exceptional paths. (Har har.)
Example: geolocation api has error callbacks whereas File API has FileError
(or some such / on bad connection and just blasting this w/o checking).
On Thu, Nov 22, 2012 at 2:01 PM, Patrick Mueller <pm...@gmail.com> wrote:
> On Thu, Nov 22, 2012 at 6:00 AM, Brian LeRoux <b...@brian.io> wrote:
>
> > ya upon further consideration making these TypeException's feels right
> > since, ideally, this error would only be seen by a plugin author and not
> > something a plugin consumer (ideally)
> >
>
> huh? I thought this was all about informing users when they pass invalid
> arguments?
>
> I'm usually in favor of "fail fast" - and so throwing an exception when you
> pass an invalid argument sounds right to me. The problem is that even
> though it's simple for us to fail fast by throwing an exception, we also
> need to make sure it's super obvious to the user that a failure has
> occurred. That's the hard part. Too many places where errors are silently
> consumed by the runtime.
>
> I think a console.log() would be appropriate - along with a thrown
> exception - lots of folks have access to a "console" these days. Or maybe
> we should come up with a new API - reportFailure() or something, which we
> could have - by default - just log to the console. For platforms that
> don't have an easily accessible console, they can override this to do
> something visible for their platform.
>
> --
> Patrick Mueller
> http://muellerware.org
>
Re: Type checking in Cordova JS plugins
Posted by Patrick Mueller <pm...@gmail.com>.
On Thu, Nov 22, 2012 at 6:00 AM, Brian LeRoux <b...@brian.io> wrote:
> ya upon further consideration making these TypeException's feels right
> since, ideally, this error would only be seen by a plugin author and not
> something a plugin consumer (ideally)
>
huh? I thought this was all about informing users when they pass invalid
arguments?
I'm usually in favor of "fail fast" - and so throwing an exception when you
pass an invalid argument sounds right to me. The problem is that even
though it's simple for us to fail fast by throwing an exception, we also
need to make sure it's super obvious to the user that a failure has
occurred. That's the hard part. Too many places where errors are silently
consumed by the runtime.
I think a console.log() would be appropriate - along with a thrown
exception - lots of folks have access to a "console" these days. Or maybe
we should come up with a new API - reportFailure() or something, which we
could have - by default - just log to the console. For platforms that
don't have an easily accessible console, they can override this to do
something visible for their platform.
--
Patrick Mueller
http://muellerware.org
Re: Type checking in Cordova JS plugins
Posted by Brian LeRoux <b...@brian.io>.
ya upon further consideration making these TypeException's feels right
since, ideally, this error would only be seen by a plugin author and not
something a plugin consumer (ideally)
On Wed, Nov 21, 2012 at 8:53 PM, Michal Mocny <mm...@chromium.org> wrote:
> On Wed, Nov 21, 2012 at 3:51 PM, Andrew Grieve <ag...@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 <ag...@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.
> >> >
> >>
>
Re: Type checking in Cordova JS plugins
Posted by Michal Mocny <mm...@chromium.org>.
On Wed, Nov 21, 2012 at 3:51 PM, Andrew Grieve <ag...@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 <ag...@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.
>> >
>>
Re: Type checking in Cordova JS plugins
Posted by Andrew Grieve <ag...@chromium.org>.
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 <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 <ag...@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.
> >
>
Re: Type checking in Cordova JS plugins
Posted by Brian LeRoux <b...@brian.io>.
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 <pu...@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 <ag...@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.
>
Re: Type checking in Cordova JS plugins
Posted by Jesse MacFadyen <pu...@gmail.com>.
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 <ag...@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.