cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jesse <purplecabb...@gmail.com>
Subject Re: Major refactoring of Plugman and CLI
Date Tue, 24 Sep 2013 21:50:29 GMT
Oops, I missed that Braden, thanks.

@Ryan, the code is committed, but won't be pushed to npm for at least a
week.
We will have a chance to run it through the rigors first, although it is
seeming like there will be little to no impact.



@purplecabbage
risingj.com


On Tue, Sep 24, 2013 at 1:22 PM, Ryan Stewart <rstewart@adobe.com> wrote:

> Apologies as I'm having a little bit of trouble following this
> conversation. Is this change already in Cordova CLI? Is it part of the
> 3.1.0 release?
>
> =Ryan
>
> On 9/24/13 1:18 PM, "Jesse" <purplecabbage@gmail.com> wrote:
>
> >So, in the phonegap-cli, which uses cordova-cli, we have this call:
> >
> >var cordova = require('cordova');
> >...
> >cordova.create(options.path, options.id, options.name, function(e) { ...
> >
> >
> >So this is definitely broken now as a result of this change, and has to be
> >re-written to support promises.
> >
> >Couldn't you have maintained support for callbacks, by doing this?
> >
> >module.exports = function create (dir, id, name, callback) {
> >var retQ = Q();
> >    if(callback) {
> >         // output callback deprecation notice
> >         retQ.then(callback);
> >    }
> >    return retQ;
> >}
> >
> >Not that I like lingering potentially unused functionality, but short of
> >knowing ahead that this is going to change, it is hard to prepare for.
> >
> >
> >
> >
> >@purplecabbage
> >risingj.com
> >
> >
> >On Tue, Sep 24, 2013 at 12:21 PM, purplecabbage
> ><purplecabbage@gmail.com>wrote:
> >
> >> Thanks for clarifying Braden.
> >> My general rule is if I am changing many files, I will run it by the
> >>list.
> >> Also if I am touching other platforms, I will always consult the list.
> >>  Luckily/unluckily, many of my drastic changes have happened in the
> >>win/wp
> >> area where there are far fewer hands in the pot.
> >>
> >> Sent from my iPhone
> >>
> >> > On Sep 24, 2013, at 11:49 AM, Braden Shepherdson <braden@chromium.org
> >
> >> wrote:
> >> >
> >> > I get the impression that "talking about it internally" came out
> >>wrong.
> >> > Here's what actually happened:
> >> >
> >> > - I started out to fix some bugs, including killing use of shell.exec
> >> > because it leaks filehandles and sometimes causes fatal errors.
> >> > - I adopted promises to control the horribly deep callback trees we
> >>were
> >> > building up.
> >> > - That improved the code so much that it spread to the rest of the
> >>tools
> >> > and became a large refactoring.
> >> > - I was worried that I now needed to bring this up with the list and
> >>get
> >> > buy-in, since it had become bigger than fixing a few bugs.
> >> > - On consulting with the others who happened to be in the room at the
> >> time,
> >> > the feeling was that since the outside API hadn't changed, this was
> >>fine.
> >> > Bug fixes and code cleanup don't need buy-in from everyone. However, I
> >> > should definitely announce the change.
> >> > - Here we are.
> >> >
> >> > I guess I haven't found the balance of what should be done silently,
> >>what
> >> > announced as it goes out, and what discussed ahead of time. Since the
> >> > feeling is that this should have been mentioned in advance, I will
> >>adjust
> >> > in the future.
> >> >
> >> > Braden
> >> >
> >> >> On Tue, Sep 24, 2013 at 2:20 PM, Shazron <shazron@gmail.com>
wrote:
> >> >>
> >> >> Definitely. Talking about changes is cool, not talking about it --
> >>not
> >> >> cool.
> >> >>
> >> >>
> >> >> On Tue, Sep 24, 2013 at 11:01 AM, purplecabbage <
> >> purplecabbage@gmail.com
> >> >>> wrote:
> >> >>
> >> >>> Q is a good choice. Not talking about it was absolutely the wrong
> >> choice.
> >> >>> There are no internal teams , there is only this list.
> >> >>>
> >> >>> Sent from my iPhone
> >> >>>
> >> >>>>> On Sep 24, 2013, at 6:48 AM, Braden Shepherdson
> >><braden@chromium.org
> >> >
> >> >>>> wrote:
> >> >>>>
> >> >>>> We debated internally at Google how much to talk about this.
In the
> >> end
> >> >>> we
> >> >>>> decided that since the external APIs were not changing, this
could
> >>be
> >> >>>> claimed as an internal refactoring. I'm not sure whether that
was
> >>the
> >> >>> right
> >> >>>> call.
> >> >>>>
> >> >>>> About fetch and platforms, to be clear, those are far from
the only
> >> >>> modules
> >> >>>> that have changes, they're just the examples I chose. Using
almost
> >>any
> >> >> of
> >> >>>> the internal modules directly will require refactoring.
> >> >>>>
> >> >>>> Braden
> >> >>>>
> >> >>>>
> >> >>>>> On Tue, Sep 24, 2013 at 6:56 AM, Anis KADRI <anis.kadri@gmail.com
> >
> >> >>> wrote:
> >> >>>>>
> >> >>>>> cool.
> >> >>>>>
> >> >>>>> I don't think we're using fetch/platforms directly.
> >> >>>>>
> >> >>>>> -a
> >> >>>>>
> >> >>>>>> On Tue, Sep 24, 2013 at 11:35 AM, Brian LeRoux <b@brian.io>
> >>wrote:
> >> >>>>>> Kewl. I'm down and happen to really like Q. Not sure
everyone
> >>will
> >> >>> agree.
> >> >>>>>> Maybe next time a heads up to the list so we can discuss
arch
> >> changes
> >> >>>>> like
> >> >>>>>> this.
> >> >>>>>>
> >> >>>>>>
> >> >>>>>> On Mon, Sep 23, 2013 at 8:13 PM, Braden Shepherdson
<
> >> >>> braden@chromium.org
> >> >>>>>> wrote:
> >> >>>>>>
> >> >>>>>>> Whoops, I forgot to mention, I created and pushed
a
> >>cordova-3.1.x
> >> >>>>> branch of
> >> >>>>>>> both tools before merging this; fixes for the 3.1.0
release
> >>should
> >> >> be
> >> >>> in
> >> >>>>>>> there. I don't intend to launch the refactored
code to NPM until
> >> >> we've
> >> >>>>> had
> >> >>>>>>> at least a week of trying it out.
> >> >>>>>>>
> >> >>>>>>> Braden
> >> >>>>>>>
> >> >>>>>>>
> >> >>>>>>> On Mon, Sep 23, 2013 at 2:08 PM, Braden Shepherdson
<
> >> >>>>> braden@chromium.org
> >> >>>>>>>> wrote:
> >> >>>>>>>
> >> >>>>>>>> tl;dr: Plugman and CLI now uses Q.js[1] Promises
internally
> >> instead
> >> >>> of
> >> >>>>>>>> callbacks. This has significantly clarified
and shortened the
> >> code.
> >> >>>>> The
> >> >>>>>>>> public API (plugman.fetch, cordova.platform,
etc.) HAVE NOT
> >> >> changed!
> >> >>>>>>>>
> >> >>>>>>>> If you use CLI on the command line, nothing
has changed.
> >> >>>>>>>>
> >> >>>>>>>> If you downstream CLI and/or Plugman, but use
cordova.foo and
> >> >>>>>>> plugman.foo,
> >> >>>>>>>> nothing has changed (except possibly that a
few calls are a bit
> >> >> more
> >> >>>>>>> async
> >> >>>>>>>> than before, so code that cheats and pretends
they're sync
> >>might
> >> >> fail
> >> >>>>>>> now).
> >> >>>>>>>>
> >> >>>>>>>> If you downstream either one, but require internal
modules like
> >> >>>>> fetch.js
> >> >>>>>>>> or platform.js directly, you should stop doing
that and use
> >> >>>>> plugman.fetch
> >> >>>>>>>> etc. instead. If you want to continue calling
them directly,
> >> you'll
> >> >>>>> need
> >> >>>>>>> to
> >> >>>>>>>> port to use promises.
> >> >>>>>>>>
> >> >>>>>>>> If you've been working on Plugman or CLI and
I just broke
> >> >> everything,
> >> >>>>>>> feel
> >> >>>>>>>> free to yell at me on IRC (#cordova, shepheb)
or Gtalk (braden
> >>at
> >> >>>>> google
> >> >>>>>>>> dot com) or email. It's not hard to port things
to handle
> >>promises
> >> >>>>> (see
> >> >>>>>>>> below), and their basic use is not hard to
understand (see the
> >> >>>>>>> tutorial[1]).
> >> >>>>>>>>
> >> >>>>>>>> If you really do need to port something, and
you used to do a
> >> >>> function
> >> >>>>>>>> call like this:
> >> >>>>>>>>
> >> >>>>>>>> whateverFunc(args..., function(err){
> >> >>>>>>>> if (err) {
> >> >>>>>>>>   foo
> >> >>>>>>>> } else {
> >> >>>>>>>>   bar
> >> >>>>>>>> }
> >> >>>>>>>> });
> >> >>>>>>>>
> >> >>>>>>>> the correct call is now:
> >> >>>>>>>>
> >> >>>>>>>> whateverFunc(args...).done(function() {
> >> >>>>>>>> bar
> >> >>>>>>>> }, function(err) {
> >> >>>>>>>> foo
> >> >>>>>>>> });
> >> >>>>>>>>
> >> >>>>>>>>
> >> >>>>>>>> [1] Q.js tutorial at https://github.com/kriskowal/q
> >> >>
> >>
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message