cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From purplecabbage <purplecabb...@gmail.com>
Subject Re: [JS] merging to master for cordova-js and cordova-plugman
Date Sat, 26 Apr 2014 02:20:15 GMT
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?
> 
> 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
>> 

Mime
View raw message