Knowing the implication of your changes is pretty critical. IMHO everyone who commits to the generic portions of cordova-js should be prepared to at least smoke test in WP7or8, Android, iOS, and BB. Or at a bare minimum have extreme confidence that your change will not affect them.
On Wed, Feb 20, 2013 at 1:07 PM, Andrew Grieve <agri...@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 <mich...@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 <f...@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" <agri...@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 <agri...@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 > > > >><agri...@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 > > > >>><agri...@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 > > > >>>><agri...@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 <f...@adobe.com> > wrote: > > > >>>>> > > > >>>>>> This all seems reasonable. Shall we start a branch? > > > >>>>>> > > > >>>>>> On 1/15/13 2:47 PM, "Andrew Grieve" <agri...@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 > > > >>>>>> > > > > >>>>>> >* > > > >>>>>> > > > >>>>>> > > > >>>>> > > > >>>> > > > >>> > > > >> > > > > > > > > > -- @purplecabbage risingj.com
