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