Mailing-List: contact dev-help@cordova.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cordova.apache.org
Received-SPF: pass (nike.apache.org: domain of anis.kadri@gmail.com designates
 209.85.160.46 as permitted sender)
MIME-Version: 1.0
In-Reply-To: 
 <CABiQX1WAo22m4V4iQACPxyMcDXR18uEqCwzjxRbrq_m7VHQo_Q@mail.gmail.com>
References: 
 <CANpVHHxaZzqVpVHqFjgSnzvqEBix4vhznwSj9frwtXzE=C=AvA@mail.gmail.com>
	<CADVgkyPSv-a_2NR31PBsF7XypEG3FELjemK-iStWBMRG=Nw1ng@mail.gmail.com>
	<CABiQX1X6u0yNkV=XZNzf17vWBbfwjOEdkOaaeniQykrhDteOyg@mail.gmail.com>
	<CAEeF2Tcp0+6THROa7vEKapPSDqjb8in=94BKzspUdw-cgC=EyA@mail.gmail.com>
	<CAEeF2Tdp+FoEc+=vZkToGMH1sSHx7y064_zFjyFUOqArtFh3MA@mail.gmail.com>
	<CAEeF2TeWwGn1ypb4cLNWnGNgyiG83xS+qO9GzaFG=4HuO7E_0Q@mail.gmail.com>
	<CABiQX1V+NjY5zBiYHs_01wV95GAmTQ_3XdHRSKm_8D39TAt-jg@mail.gmail.com>
	<CANpVHHy4b_QMfeoTKJB5bySBgwX0sBxFWVYwT0CTUPNtUM8TyA@mail.gmail.com>
	<CAEeF2TcLqM9jOtbRAE7b2MwnhARgQXOMP6dnq0BcEXG-O=iNmQ@mail.gmail.com>
	<CABiQX1WAo22m4V4iQACPxyMcDXR18uEqCwzjxRbrq_m7VHQo_Q@mail.gmail.com>
Date: Fri, 25 Apr 2014 17:40:40 -0700
Message-ID: 
 <CANpVHHxYT02u42fx9YutArX_Ak=LYuFoppeU-wiurkQ9cwQ_fg@mail.gmail.com>
Subject: Re: [JS] merging to master for cordova-js and cordova-plugman
From: Anis KADRI <anis.kadri@gmail.com>
To: Andrew Grieve <agrieve@chromium.org>
Cc: dev <dev@cordova.apache.org>, Michal Mocny <mmocny@chromium.org>
Content-Type: multipart/alternative; boundary=047d7bd8fc22e617d704f7e754f0

--047d7bd8fc22e617d704f7e754f0
Content-Type: text/plain; charset=UTF-8

On Fri, Apr 25, 2014 at 11:05 AM, Andrew Grieve <agrieve@chromium.org>wrote:

> On Fri, Apr 25, 2014 at 1:35 PM, Michal Mocny <mmocny@chromium.org> wrote:
> > On Fri, Apr 25, 2014 at 1:27 PM, Anis KADRI <anis.kadri@gmail.com>
> wrote:
> >
> >> Contacts is known to not work because some modules
> >> require('./ContactError') instead of
> >> ('org.apache.cordova.contacts.ContactError') like most other modules
> do. I
> >> didn't bother to fix yet it because it was an exception not a rule.
> >> I didn't try any of the chrome plugins. They might be doing the same
> sort
> >> of thing.
>
> Just wanted to be clear here that "fix" means allow the "./syntax",
> not convert to abs syntax. File plugin uses this extensively, and
> we've supported it for quite a while.
>

Thanks for the heads up but require('path') works. The only difference is
that it actually checks if the path exists and if it doesn't then it fails.
Which makes this file [1] fail because it's requiring a module that is
present in the parent directory (../ContactError) while referencing it as
if it was in the same directory (./ContactError). Are chrome plugins doing
a similar thing ? File/FileTransfer are not concerned because the paths
they reference do exist.


>
>
> >> I didn't noticed any major performance issues when comparing old plugman
> >> and new plugman with browserify. I am not using CLI though.
>
> As a litmus test, I'd suggest running the createmobilespec.js script
> within mobile-spec.
>

I will try this out. The reason it takes longer is partly because it's
generating 2 bundles. I am sure there can be ways to improve performance
though. I will keep looking into it.


>
> >> Thanks for giving me the time to look into this before reverting my
> >> changes. Much appreciated. Besides reverting cordova-js was completely
> >> unnecessary because I only added a task to compile a browserify bundle
> >> (everything else was not touched).
>
> I think it's important to keep the continuous build green, and
> cordova-js was broken:
> https://issues.apache.org/jira/browse/CB-6515
>
> To re-do your changes, just git revert the revert commit and carry on.
> It's important that everyone else isn't blocked by master being
> broken.
>

jshint was failling because I used 2 indentations instead of 4. Indeed,
that is absolutely unacceptable.
I suggest that we make indentation our top priority going forward. Those
types of bug that users report that make them unable to compile their
projects can just sit in there and wait after all
(CB-6441<https://issues.apache.org/jira/browse/CB-6441>).
Who cares about real users as long as the continuous build screen is green,
right?

I fixed the indentation which makes jshint happy. I merged the browserify
branch back to master (with the critical indentation fix). You do let me
know if your continuous build is bleeding again.

Finally, you asked me at the last hangout to merge everything to master in
order to survive the breakout of cordova-cli and cordova-plugman and that
is why I took the liberty to do it sooner rather than later. Also we need
to start testing this stuff at some point and it's not like nothing has
ever been broken with the project.

[1]
https://git-wip-us.apache.org/repos/asf?p=cordova-plugin-contacts.git;a=blob;f=www/ios/Contact.js;h=b40c41acc5c78e2233ca434388ab012cd635fba9;hb=HEAD


>
> >>
> >
> > All the fixes will probably take more than an hour, and shouldn't block
> > master.  Why the rush to merge in?
> >
> >
> >>
> >>
> >>
> >> On Fri, Apr 25, 2014 at 8:13 AM, Andrew Grieve <agrieve@chromium.org
> >wrote:
> >>
> >>> reverted the merge commits to plugman & js.
> >>>
> >>> On Fri, Apr 25, 2014 at 9:43 AM, Michal Mocny <mmocny@chromium.org>
> >>> wrote:
> >>> > Also Anis, would it please be possible to squash most of these
> commits
> >>> into
> >>> > a few units of standalone work?  Probably best way to do that is
> create
> >>> a
> >>> > new local branch from your old branch point, apply the diffs from
> >>> > browserify branch, rebase -i them, then push that branch (this way
> you
> >>> > don't need to push with force).
> >>> >
> >>> > -Michal
> >>> >
> >>> >
> >>> > On Fri, Apr 25, 2014 at 9:30 AM, Michal Mocny <mmocny@chromium.org>
> >>> wrote:
> >>> >
> >>> >>
> >>> >>
> >>> >>
> >>> >> On Fri, Apr 25, 2014 at 12:53 AM, Michal Mocny <mmocny@chromium.org
> >>> >wrote:
> >>> >>
> >>> >>> TLDR; Ran a bunch of experiments tonight.  I think this is too
> early
> >>> to
> >>> >>> merge into master.  We pointed out several issues in previous
> threads
> >>> and
> >>> >>> seems they were ignored.
> >>> >>>
> >>> >>> Few quick comments from trying this:
> >>> >>> - cordova-js is a new dependency of plugman, and needs to be npm
> >>> linked
> >>> >>> to local dev version
> >>> >>> - Seems we run browserify after each plugin add (possibly due to an
> >>> >>> auto-prepare?) so creating projects like mobilespec or any mobile
> >>> chrome
> >>> >>> app is now *much* slower (measured in minutes)
> >>> >>>   - Each cordova prepare now takes 6.5s on a very small project :'(
> >>> >>>
> >>> >> (reverted changes locally, old prepare takes <0.5s on same project,
> >>> which
> >>> >> is still slow!)
> >>> >>
> >>> >>
> >>> >>> Things that are currently found broken:
> >>> >>> - prepare step fails for my cordova testing application after
> >>> >>> installing org.apache.cordova.contacts, and
> >>> >>> - prepare step fails for *all* cca apps because of same error as
> >>> above,
> >>> >>> but for chrome.runtime plugin :(
> >>> >>>   - These issues seem due to js-modules not being browserify-ed
> >>> properly.
> >>> >>>  It may be that both are bad modules (?), but it used to work fine!
> >>> >>>
> >>> >>> I did get a few apps running fine, so at least we got that going
> for
> >>> us ;)
> >>> >>>
> >>> >>> Still to do:
> >>> >>> - track impact tp startup time
> >>> >>> - see if there aren't any plugins with subtle bugs due to auto-runs
> >>> >>> behaviour
> >>> >>>
> >>> >>> -Michal
> >>> >>>
> >>> >>>
> >>> >>> On Thu, Apr 24, 2014 at 9:34 PM, Andrew Grieve <
> agrieve@chromium.org
> >>> >wrote:
> >>> >>>
> >>> >>>> Cool! Does no impact mean that browserify is still not used by
> >>> >>>> default, or does it mean that it's backward compatible?
> >>> >>>>
> >>> >>>> Failing specs sounds like impact...
> >>> >>>>
> >>> >>>> And it does look like medic is failing due to browserify-type
> things:
> >>> >>>> http://108.170.217.131:8010/waterfall
> >>> >>>>
> >>> >>>> Unless you feel like powering through this tonight, I'll probably
> >>> >>>> revert in the morning so that our continuous build can stay green.
> >>> >>>>
> >>> >>>> On Thu, Apr 24, 2014 at 6:06 PM, Brian LeRoux <b@brian.io> wrote:
> >>> >>>> > \o/
> >>> >>>> >
> >>> >>>> >
> >>> >>>> > On Thu, Apr 24, 2014 at 2:30 PM, Anis KADRI <anis@apache.org>
> >>> wrote:
> >>> >>>> >
> >>> >>>> >> I just merged both browserify branches into master. There
> should
> >>> be no
> >>> >>>> >> impact.
> >>> >>>> >> Right now most specs pass expect for File, FileTransfer, Media
> and
> >>> >>>> Contacts
> >>> >>>> >> due to some issues with merges/clobbers and I am looking into
> >>> those.
> >>> >>>> >>
> >>> >>>> >> Also, I got rid of the project cache condition in plugman that
> was
> >>> >>>> >> preventing iOS frameworks from being added (CB-6441)
> >>> >>>> >>
> >>> >>>> >> Anis
> >>> >>>> >>
> >>> >>>>
> >>> >>>
> >>> >>>
> >>> >>
> >>>
> >>
> >>
>

--047d7bd8fc22e617d704f7e754f0--