cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Grieve <agri...@chromium.org>
Subject Re: [JS] merging to master for cordova-js and cordova-plugman
Date Tue, 29 Apr 2014 16:56:27 GMT
On Fri, Apr 25, 2014 at 8: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.
>

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