cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Grieve <agri...@chromium.org>
Subject Re: Symbol Mapping in Cordova
Date Thu, 21 Feb 2013 19:24:36 GMT
I think so! Thanks everyone for working through this with me.


On Thu, Feb 21, 2013 at 2:08 PM, Filip Maj <fil@adobe.com> wrote:

> Hope the conclusions in this thread are satisfactory.
>
> Andrew, I apologize for the tense tone in my emails yesterday. I should
> have been more chill about it.
>
> On 2/20/13 7:52 PM, "Michael Brooks" <michael@michaelbrooks.ca> wrote:
>
> >Sounds awesome Andrew, thanks for the concise recap.
> >
> >I agree, JIRA isn't the ideal solution when considering today's options
> >around code conversation, but it's what we have as an Apache project.
> >C'est
> >le vie.
> >
> >Regardless, if you need a platform to do a task, then JIRA is the answer.
> >If you need to discuss something before deciding on the tasks to do, then
> >the mailing-list is the answer.
> >
> >Michael
> >
> >On Wed, Feb 20, 2013 at 1:58 PM, Andrew Grieve <agrieve@chromium.org>
> >wrote:
> >
> >> Recap:
> >> I tested the common changes against iOS & Android and checked them in. I
> >> also checked in the blackberry ones, since they contain better test
> >> coverage in cordova-js than other platforms.
> >>
> >> I then emailed out asking if anyone could test the remaining changes on
> >>the
> >> branch against WP and webos. After 8 days of silence, I merged them in
> >> hopes that sometime between now and the 2.6 release someone will run the
> >> tests and discover if the changes broke anything.
> >>
> >>
> >> Outcome:
> >> -Instead of the mailing-list, we'll use JIRA issues to assign specific
> >> owners to the job of testing out these changes when they come up.
> >>
> >>
> >> What to do now:
> >> I'll create two sub-tasks of CB-2227. One for WP, one for webos.
> >>
> >>
> >> Sound right / good?
> >>
> >>
> >>
> >>
> >>
> >> On Wed, Feb 20, 2013 at 4:32 PM, Michal Mocny <mmocny@chromium.org>
> >>wrote:
> >>
> >> > Agree with where this conversation is going.  I do think a "call to
> >> action"
> >> > for review is important (esp given the number of platforms) and
> >>perhaps
> >> > JIRA isn't the absolute worst way to do it ;)
> >> >
> >> > Another question: Are there plans to expand CI to other platforms?
> >>  Support
> >> > requesting tests for a remote feature branch?  This may remove some
> >> strain
> >> > on testers for the less popular platforms.
> >> >
> >> >
> >> > On Wed, Feb 20, 2013 at 4:29 PM, Filip Maj <fil@adobe.com> wrote:
> >> >
> >> > > Good call Mike. Moving this sort of stuff to JIRA (and bringing
> >>back to
> >> > > list when necessary) makes a lot of sense.
> >> > >
> >> > > On 2/20/13 1:27 PM, "Michael Brooks" <michael@michaelbrooks.ca>
> >>wrote:
> >> > >
> >> > > >>
> >> > > >> If the "can you guys test my changes" answer is "no", then
it'd
> >>be
> >> > > >>great to
> >> > > >> hear a "no" instead of 8 days of silence. That said, I think
> >>we'd be
> >> > > >>able
> >> > > >> to move faster if we just took some time to review/test each
> >>others'
> >> > > >> changes when necessary. We do this when processing pull requests
> >> from
> >> > > >> external devs, so why not do this for each other?
> >> > > >
> >> > > >
> >> > > >Totally valid and I support the suggestion of code reviews (as
> >>long as
> >> > its
> >> > > >not a hinderance to commit ease).
> >> > > >
> >> > > >It would be nice if we can bring conversations closer to the code
> >>(via
> >> > > >GitHub pull requests and code comments) instead of blasting emails
> >>at
> >> > each
> >> > > >other. I assume using GitHub more is against the Apache Way? We
> >>could
> >> > > >bring
> >> > > >conversations more into to JIRA. For this particular example,
> >>Andrew
> >> > could
> >> > > >create a JIRA issue and assign subtasks for the major platforms
to
> >> test
> >> > to
> >> > > >branch and report back any issues.
> >> > > >
> >> > > >Michael
> >> > > >
> >> > > >On Wed, Feb 20, 2013 at 1:07 PM, Andrew Grieve
> >><agrieve@chromium.org>
> >> > > >wrote:
> >> > > >
> >> > > >> I agree with your sentiments, but I think it's impractical
in
> >> > practice.
> >> > > >>We
> >> > > >> have ~11 platforms, and any change to common js affects them
all.
> >> > > >>
> >> > > >> In this case, I would need to learn how to build & run
on webos,
> >> > tizen,
> >> > > >>wp7
> >> > > >> and windowphone, as well as buying the required hardware
to do
> >>so. A
> >> > > >>tall
> >> > > >> order for some refactoring changes.
> >> > > >>
> >> > > >> If the "can you guys test my changes" answer is "no", then
it'd
> >>be
> >> > > >>great to
> >> > > >> hear a "no" instead of 8 days of silence. That said, I think
> >>we'd be
> >> > > >>able
> >> > > >> to move faster if we just took some time to review/test each
> >>others'
> >> > > >> changes when necessary. We do this when processing pull requests
> >> from
> >> > > >> external devs, so why not do this for each other?
> >> > > >>
> >> > > >>
> >> > > >>
> >> > > >>
> >> > > >>
> >> > > >>
> >> > > >> On Wed, Feb 20, 2013 at 3:47 PM, Michael Brooks
> >> > > >><michael@michaelbrooks.ca
> >> > > >> >wrote:
> >> > > >>
> >> > > >> > Agreed. Please take responsibility and test your code
on
> >>devices
> >> > > >>(ideally
> >> > > >> > not simulators).
> >> > > >> >
> >> > > >> > If your change impacts multiple platforms, have it tested
on
> >>those
> >> > > >>before
> >> > > >> > pushing to master.
> >> > > >> >
> >> > > >> > On Wed, Feb 20, 2013 at 12:42 PM, Filip Maj <fil@adobe.com>
> >> wrote:
> >> > > >> >
> >> > > >> > > Andrew did you test it on a device? Don't think
"hey guys can
> >> you
> >> > > >>test
> >> > > >> my
> >> > > >> > > changes" is a sustainable approach
> >> > > >> > >
> >> > > >> > > On 2/20/13 12:19 PM, "Andrew Grieve" <agrieve@chromium.org>
> >> > wrote:
> >> > > >> > >
> >> > > >> > > >Okay, I've interpreted the silence as a "just
go ahead and
> >> merge
> >> > it
> >> > > >> and
> >> > > >> > > >people will complain if it's broken".
> >> > > >> > > >
> >> > > >> > > >Seeing as we've now cut the next branch, I
figured it was a
> >> good
> >> > > >>time
> >> > > >> to
> >> > > >> > > >merge these remaining changes in. So, I did.
Please let me
> >>know
> >> > if
> >> > > >> > symbols
> >> > > >> > > >aren't working.
> >> > > >> > > >
> >> > > >> > > >
> >> > > >> > > >On Tue, Feb 12, 2013 at 2:39 PM, Andrew Grieve
> >> > > >><agrieve@chromium.org>
> >> > > >> > > >wrote:
> >> > > >> > > >
> >> > > >> > > >> I've just merged in most of the changes
for this
> >>(CB-2227).
> >> > > >>Again,
> >> > > >> the
> >> > > >> > > >> goal here was to move all plugin-specific
logic out of
> >>common
> >> > > >>files.
> >> > > >> > > >>It's
> >> > > >> > > >> not the end-game solution, but a step
in the right
> >>direction.
> >> > > >> > > >>
> >> > > >> > > >> There are still some changes left on the
symbolmapping
> >>branch
> >> > > >>that
> >> > > >> > > >>affect
> >> > > >> > > >> windows & webos. If there's someone
who knows how, it
> >>would
> >> be
> >> > > >>great
> >> > > >> > if
> >> > > >> > > >>you
> >> > > >> > > >> could try merging in the branch and ensure
mobile-spec is
> >> still
> >> > > >> > working
> >> > > >> > > >> with the changes. If so, these changes
can also be merged
> >> into
> >> > > >> master.
> >> > > >> > > >>
> >> > > >> > > >>
> >> > > >> > > >>
> >> > > >> > > >> On Tue, Jan 29, 2013 at 3:38 PM, Andrew
Grieve
> >> > > >> > > >><agrieve@chromium.org>wrote:
> >> > > >> > > >>
> >> > > >> > > >>> This is now finished in the branch.
There is now *no*
> >>plugin
> >> > > >>logic
> >> > > >> > left
> >> > > >> > > >>> in common.js, nor in any platform.js
files.
> >> > > >> > > >>>
> >> > > >> > > >>>
> >> > > >> > > >>>
> >> > > >> > > >>>
> >> > > >> > >
> >> > > >>
> >> > >
> >>
> https://git-wip-us.apache.org/repos/asf?p=cordova-js.git;a=shortlog;h=re
> >> > > >> > > >>>fs/heads/symbolmapping
> >> > > >> > > >>>
> >> > > >> > > >>> There is one exception, and that's
things like the "app"
> >> > plugin,
> >> > > >> > where
> >> > > >> > > >>> it's not really a plugin that a platform
can live
> >>without.
> >> > > >> > > >>>
> >> > > >> > > >>> Changes of interest:
> >> > > >> > > >>> 1. In tests, I've added helpers for
stubbing out modules
> >>&
> >> for
> >> > > >> > stubbing
> >> > > >> > > >>> out properties. I used this to be
able to undo symbol
> >> mapping
> >> > > >> within
> >> > > >> > > >>>tests.
> >> > > >> > > >>>
> >> > > >> > > >>> var propertyreplacer =
> >>require('cordova/propertyreplacer');
> >> > > >> > > >>> propertyreplacer.stub(platform, 'id',
'test');
> >> > > >> > > >>>
> >> > > >> > > >>> var modulereplacer = require('cordova/modulereplacer');
> >> > > >> > > >>> modulereplacer.replace('cordova/platform',
{id:'test',
> >> > > >> > > >>> initialize:createSpy()});
> >> > > >> > > >>>
> >> > > >> > > >>> 2. Loading plugins by name (aka, looping
through all
> >>defined
> >> > > >> modules
> >> > > >> > > >>>and
> >> > > >> > > >>> loading the ones that have names that
match a pattern).
> >> > > >> > > >>> A) "symbols" Modules that have the
name "symbols" are
> >>loaded
> >> > to
> >> > > >> > define
> >> > > >> > > >>> their plugin's module->JS symbol
mappings
> >> > > >> (merges/clobbers/defaults).
> >> > > >> > > >>>On
> >> > > >> > > >>> Blackberry, sub-platform symbol files
are called
> >> "bbsymbols".
> >> > > >> > > >>> B) "plugininit" Modules that have
the name "plugininit"
> >>are
> >> > > >>loaded
> >> > > >> to
> >> > > >> > > >>> perform any custom start-up logic.
> >> > > >> > > >>> C) "*Proxy" On Windows8, modules that
end with "Proxy"
> >>are
> >> > > >>loaded
> >> > > >> on
> >> > > >> > > >>> start-up.
> >> > > >> > > >>>
> >> > > >> > > >>> I don't love the looping-through-module-names
approach,
> >>but
> >> > > >>thought
> >> > > >> > it
> >> > > >> > > >>> was a good initial solution while
we talk about better
> >> ideas.
> >> > > >>To do
> >> > > >> > > >>>this, I
> >> > > >> > > >>> had to make the moduleMap exported,
which it wasn't
> >>before.
> >> > > >> Certainly
> >> > > >> > > >>> interested to hear if this is a really
bad idea, and what
> >> > > >> > alternatives
> >> > > >> > > >>>we
> >> > > >> > > >>> could use going forward.
> >> > > >> > > >>>
> >> > > >> > > >>>
> >> > > >> > > >>>
> >> > > >> > > >>>
> >> > > >> > > >>>
> >> > > >> > > >>>
> >> > > >> > > >>> On Thu, Jan 17, 2013 at 12:45 PM,
Andrew Grieve
> >> > > >> > > >>><agrieve@chromium.org>wrote:
> >> > > >> > > >>>
> >> > > >> > > >>>> Pushed up the change with the
File plugin being
> >>registered
> >> in
> >> > > >>this
> >> > > >> > new
> >> > > >> > > >>>> way. Please let me know if you
have concerns about it,
> >> since
> >> > > >>the
> >> > > >> > next
> >> > > >> > > >>>>step
> >> > > >> > > >>>> is moving over other plugin APIs,
which is boring work
> >>:P.
> >> > > >> > > >>>>
> >> > > >> > > >>>> Also, let's move any discussion
into the JIRA issue:
> >> > > >> > > >>>> https://issues.apache.org/jira/browse/CB-2227
> >> > > >> > > >>>>
> >> > > >> > > >>>>
> >> > > >> > > >>>>
> >> > > >> > > >>>> On Wed, Jan 16, 2013 at 4:35 PM,
Andrew Grieve
> >> > > >> > > >>>><agrieve@chromium.org>wrote:
> >> > > >> > > >>>>
> >> > > >> > > >>>>> Branch started!
> >> > > >> > > >>>>>
> >> > > >> > > >>>>> I've completed steps 1 &
2.
> >> > > >> > > >>>>>
> >> > > >> > > >>>>>
> >> > > >> > > >>>>>
> >> > > >> > > >>>>>
> >> > > >> > >
> >> > > >>
> >> https://git-wip-us.apache.org/repos/asf?p=cordova-js.git;a=shortlog;h=
> >> > > >> > > >>>>>refs/heads/symbolmapping
> >> > > >> > > >>>>>
> >> > > >> > > >>>>>
> >> > > >> > > >>>>> On Wed, Jan 16, 2013 at 1:39
PM, Filip Maj
> >><fil@adobe.com
> >> >
> >> > > >> wrote:
> >> > > >> > > >>>>>
> >> > > >> > > >>>>>> This all seems reasonable.
Shall we start a branch?
> >> > > >> > > >>>>>>
> >> > > >> > > >>>>>> On 1/15/13 2:47 PM, "Andrew
Grieve"
> >><agrieve@google.com>
> >> > > >>wrote:
> >> > > >> > > >>>>>>
> >> > > >> > > >>>>>> >Sorry to dump another
large email on the list, but
> >>I'm
> >> > > >>hoping
> >> > > >> > this
> >> > > >> > > >>>>>> one is
> >> > > >> > > >>>>>> >at least less controversial
:). I wrote up a plan for
> >> > moving
> >> > > >> > > >>>>>> >module->symbol
> >> > > >> > > >>>>>> >mapping out of common.js
& platform.js and into
> >> individual
> >> > > >> > plugins.
> >> > > >> > > >>>>>> >
> >> > > >> > > >>>>>> >If you have feedback/comments,
let me know.
> >> > > >> > > >>>>>> >
> >> > > >> > > >>>>>> >* Goals:
> >> > > >> > > >>>>>> >
> >> > > >> > > >>>>>> >   - Change from listing
module->symbol mapping
> >>within
> >> > > >> common.js
> >> > > >> > &
> >> > > >> > > >>>>>> >   platform.js, to
listing this within the plugins
> >> > > >>themselves.
> >> > > >> > > >>>>>> >   - Support apps
that don't want us to clobber
> >>global
> >> > > >>symbols.
> >> > > >> > > >>>>>> >      - aka, allow
module->symbol mapping to be
> >>turned
> >> off
> >> > > >> > > >>>>>> >   - Allow retrieval
of clobbered globals
> >> > > >> > > >>>>>> >      - Currently
modules save it themselves when
> >>they
> >> are
> >> > > >> loaded
> >> > > >> > > >>>>>> >      - This won't
work (reliably) for saving
> >>references
> >> > to
> >> > > >> > globals
> >> > > >> > > >>>>>> >      overridden by
other modules
> >> > > >> > > >>>>>> >      - This gets
in the way of the idea of
> >>lazy-loading
> >> > > >> modules
> >> > > >> > > >>>>>>via
> >> > > >> > > >>>>>> >getters
> >> > > >> > > >>>>>> >   - Support the use
of other module loaders
> >> > > >> > > >>>>>> >      - So... don't
do crazy things at require()
> >>time.
> >> > > >> > > >>>>>> >
> >> > > >> > > >>>>>> >
> >> > > >> > > >>>>>> >Requirements:
> >> > > >> > > >>>>>> >
> >> > > >> > > >>>>>> >   - Plugins must
be able to declare dependencies
> >> > > >> > > >>>>>> >   - Plugins must
be able to delay onDeviceReady()
> >> > > >> > > >>>>>> >   - Plugins must
be able to run code to initialize
> >> > > >> > > >>>>>> >
> >> > > >> > > >>>>>> >
> >> > > >> > > >>>>>> >Implementation modulemapper.js:
> >> > > >> > > >>>>>> >
> >> > > >> > > >>>>>> >   - clobbers(...)
> >> > > >> > > >>>>>> >   - merges(...)
> >> > > >> > > >>>>>> >   - defaults(...)
> >> > > >> > > >>>>>> >   - mapModules(wnd)
> >> > > >> > > >>>>>> >   - getOriginalSymbol('FileSystem')
> >> > > >> > > >>>>>> >
> >> > > >> > > >>>>>> >
> >> > > >> > > >>>>>> >Start-up flow:
> >> > > >> > > >>>>>> >
> >> > > >> > > >>>>>> >   1. Parse all modules
> >> > > >> > > >>>>>> >   2. common-bootstrap:
> >> > > >> > > >>>>>> >      1. Loads list
of modules named
> >>"cordova.*/symbols"
> >> > > >> > > >>>>>> >      2. Run modulemapper.mapModules(window);
> >> > > >> > > >>>>>> >      3. Loads list
of modules named "cordova.*/main"
> >> > > >> > > >>>>>> >
> >> > > >> > > >>>>>> >
> >> > > >> > > >>>>>> >symbols.js files:
> >> > > >> > > >>>>>> >
> >> > > >> > > >>>>>> >   - Will make calls
to modulemapper instead of
> >> exporting
> >> > > >> > > >>>>>>{clobbers:}
> >> > > >> > > >>>>>> >      - This make
dependencies work by require()ing
> >> > > >>dependent
> >> > > >> > > >>>>>>symbols
> >> > > >> > > >>>>>> >   - We want the to
be an evaluated .js file instead
> >>of
> >> > > >> something
> >> > > >> > > >>>>>> listed
> >> > > >> > > >>>>>> >in
> >> > > >> > > >>>>>> >   plugin.xml
> >> > > >> > > >>>>>> >      - So that it
can export based on browser
> >>version
> >> > > >> > > >>>>>> >
> >> > > >> > > >>>>>> >
> >> > > >> > > >>>>>> >Implementation Steps
> >> > > >> > > >>>>>> >
> >> > > >> > > >>>>>> >   1. Expose list
of registered modules in
> >> > > >>scripts/require.js
> >> > > >> so
> >> > > >> > > >>>>>>that
> >> > > >> > > >>>>>> we
> >> > > >> > > >>>>>> >   can loop over them
> >> > > >> > > >>>>>> >   2. Write modulemapper.js
(and have unit tests, of
> >> > course)
> >> > > >> > > >>>>>> >   3. Add logic to
bootstrap.js that calls into
> >> > modulemapper
> >> > > >> > > >>>>>> >   4. create $PLUGIN_NAME/symbols.js
files for each
> >> plugin
> >> > > >> within
> >> > > >> > > >>>>>> cordova.
> >> > > >> > > >>>>>> >   5. Add logic to
bootstrap.js that calls into
> >> > modulemapper
> >> > > >> > > >>>>>> >   6. Create main.js
files for those that currently
> >>have
> >> > > >>logic
> >> > > >> in
> >> > > >> > > >>>>>> their
> >> > > >> > > >>>>>> >   platform.js files
> >> > > >> > > >>>>>> >
> >> > > >> > > >>>>>> >*
> >> > > >> > > >>>>>>
> >> > > >> > > >>>>>>
> >> > > >> > > >>>>>
> >> > > >> > > >>>>
> >> > > >> > > >>>
> >> > > >> > > >>
> >> > > >> > >
> >> > > >> > >
> >> > > >> >
> >> > > >>
> >> > >
> >> > >
> >> >
> >>
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message