On Fri, Apr 25, 2014 at 10:20 PM, purplecabbage <[email protected]>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 <[email protected]> wrote: > > > > On Fri, Apr 25, 2014 at 11:05 AM, Andrew Grieve <[email protected] > >wrote: > > > >> On Fri, Apr 25, 2014 at 1:35 PM, Michal Mocny <[email protected]> > wrote: > >>>> On Fri, Apr 25, 2014 at 1:27 PM, Anis KADRI <[email protected]> > >>> 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 <[email protected] > >>>> wrote: > >>>> > >>>>> reverted the merge commits to plugman & js. > >>>>> > >>>>> On Fri, Apr 25, 2014 at 9:43 AM, Michal Mocny <[email protected]> > >>>>> 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 <[email protected] > > > >>>>>> wrote: > >>>>>> > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>>> On Fri, Apr 25, 2014 at 12:53 AM, Michal Mocny < > [email protected] > >>>>>>> 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 < > >> [email protected] > >>>>>> 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 <[email protected]> > wrote: > >>>>>>>>>> \o/ > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> On Thu, Apr 24, 2014 at 2:30 PM, Anis KADRI <[email protected]> > >>>>> 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 > >> >
