cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Brian LeRoux...@brian.io>
Subject Re: Type checking in Cordova JS plugins
Date Thu, 22 Nov 2012 11:00:12 GMT
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 <mmocny@chromium.org> wrote:

> 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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message