cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michal Mocny <mmo...@chromium.org>
Subject Re: [JS] merging to master for cordova-js and cordova-plugman
Date Sat, 26 Apr 2014 02:35:50 GMT
On Fri, Apr 25, 2014 at 10:20 PM, purplecabbage <purplecabbage@gmail.com>wrote:

> I believe the plan was, merge now to save headaches later *but* also hide
> the browserify functionality behind a flag, so it could co-exist.
>
> Sent from my iPhone
>
> > On Apr 25, 2014, at 5:40 PM, Anis KADRI <anis.kadri@gmail.com> wrote:
> >
> > 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?
>


Why did you include that fix as part of this unrelated work?  If you
squashed the browserify work it would have been easier to pick out more
specific reverts.  (additionally there is duplication of all the browserify
commits if you look at the git log, not sure how that happened, so revert
of anything but the whole ).


> >
> > 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.
>

Dude its not "your" CI, its our CI.  It catches real issues weekly, and
cannot do that if its red.


> >
> > 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.
>

^ What Jesse said.


>  >
> > [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
> >>
>

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