On Fri, Apr 25, 2014 at 8: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. > Looked into this. Require paths resolve at runtime, and since ios/Contact.js is listed in plugin.xml as: <js-module src="www/ios/contacts.js" name="contacts-ios"> It ends up with a module name of "org.apache.cordova.contacts.contacts-ios". So, I think if you're trying to resolve at build time, you'd need to parse the <js-module> tags to build their module ID properly, and then resolve require paths based on their module IDs instead of their paths. > > >> >> >> >> 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 >> >>> >>>> >> >> >>> >>>> >> >>> >>> >> >>> >>> >> >>> >> >> >>> >> >> >> >> >> > >
