Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cordova-lib refactoring proposal #12

Merged
merged 2 commits into from Nov 13, 2015

Conversation

@vladimir-kotikov
Copy link
Member

@vladimir-kotikov vladimir-kotikov commented Jul 13, 2015

This PR is an updated version of #9 with addressed notes

The proposal describes the way how we can improve current cordova lib design and interaction between cordova lib and platforms. The document contains high-level descriptions of proposed interfaces only. For more details on methods signatures see *.js files in this pull request.

The most important points of this proposal are:

1. The PlatformApi class

PlatformApi (or CordovaPlatform) class - an abstraction around particular platform that exposes all the actions for this platform (such as build/run), so they're accessible programmatically. It also knows how to install/uninstall plugins with all source files, web assets and js files, so this is no more responsibility of cordova-lib. It also exposes single 'prepare' method to provide a way for cordova-lib to apply project's settings/www content to platform.

The PlatformApi class needs to be implemented by each platform, that wants to use new API flow. For those platforms, which doesn't provide own PlatformApi, there will be a polyfill in cordova-lib.

There is a PR to cordova-lib, which implements the polyfill: https://github.com/MSOpenTech/cordova-lib/pull/2

More detailed info: Cordova-lib refactoring proposal.md
Interface reference: PlatformApi.js

2. The ProjectAPI class

ProjectAPI (or CordovaProject) class - an abstraction of cordova project with multiple platforms/plugins installed. Manages the platforms and delegates the responsibilities to corresponding platform:

Project.run -> this.platforms.foreach(platform.run);

It also does some additional job:

  • fetches plugins/platforms from any sources (git/npm/web)
  • resolves plugin dependencies
  • handles platform's restrictions ( tag)

More detailed info: Cordova-lib refactoring proposal.md
Interface reference: ProjectApi.js

cordova-common module

'cordova-common' module - required to share similar code between platforms and lib. Currently there are following candidates for moving to cordova-common:

`superspawn`
`ActionStack`
`CordovaError`
`PlatformJson`
`ConfigParser`
`PluginInfo`
`PluginInfoProvider`
`ConfigChanges`
`ConfigKeeper`
`ConfigFile`
`mungeUtil`
`xmlHelpers`

The PR for cordova-common module is ready and published here: https://github.com/MSOpenTech/cordova-lib/pull/3.

Links:

PlatformApi proposal with APIs' definitions: #9
PlatformApi polyfill implementation for cordova-lib: https://github.com/MSOpenTech/cordova-lib/pull/2
cordova-common implementation: https://github.com/MSOpenTech/cordova-lib/pull/3

Pull requests:


#### Plugin management

PlatformApi.prototype.addPlugin = function (plugin, installOptions) {};

This comment has been minimized.

@omefire

omefire Jul 16, 2015
Contributor

plugin -> pluginInfo


##### Platform's 'actions'

PlatformApi.prototype.build = function(buildOptions) {};

This comment has been minimized.

@omefire

omefire Jul 16, 2015
Contributor

Shouldn't 'emulate' should also be part of this action list ?

This comment has been minimized.

@vladimir-kotikov

vladimir-kotikov Jul 16, 2015
Author Member

emulate is basically is the same as run({emulator: true}). IMO there is no sense in adding separate method as a shortcut for one option.

This comment has been minimized.

@omefire

omefire Jul 16, 2015
Contributor

cool.


Installs/uninstalls a plugin into platform. These methods handles all the files, shipped by plugin (sources, libs, assets, js-files). Accepts a `PluginInfo` instance that represents plugin that will be installed/uninstalled, and an `options` object.

__NOTE:__ The methods doesn't resolve the dependencies of plugin. Resolving dependencies is still the responsibility of caller.

This comment has been minimized.

@omefire

omefire Jul 16, 2015
Contributor

In my opinion, dependencies resolution should be done by PlatformApi.addPlugin/removePlugin.
I'm curious as to why it should be moved to the caller ?

This comment has been minimized.

@vladimir-kotikov

vladimir-kotikov Jul 16, 2015
Author Member

This will require adding a lot of duplicated code to all platforms, that responsible for fetching/downloading plugins, detecting and resolving dependencies, etc.

I believe this needs to be discussed on Hangout today.

@Laguana
Copy link

@Laguana Laguana commented Aug 17, 2015

You mention that the PlatformAPI will have a getPlatformInfo function that returns "the file structure and other properties". What exactly does this include? Does this include "the build output", and what notion is that? E.g. the iOS build output is a .app folder and maybe a plist file, while the windows build output is (a collection of) .appx or .appxbundle files?

@vladimir-kotikov
Copy link
Member Author

@vladimir-kotikov vladimir-kotikov commented Aug 18, 2015

The definition of this class is here (decided to rename PlatformInfo -> CordovaPlatform): https://github.com/vladimir-kotikov/cordova-discuss/blob/cordova-lib-refactoring/proposals/CordovaPlatform.js

The location of build artifacts is intended to be returned as a result of PlatformApi.build promise. See https://github.com/cordova/cordova-discuss/pull/12/files#diff-d6510734f8f78c73f89b7afc68707d85R151

@stevengill
Copy link
Contributor

@stevengill stevengill commented Aug 20, 2015

This looks good to me. What are the next steps. I'd like to help

@vladimir-kotikov
Copy link
Member Author

@vladimir-kotikov vladimir-kotikov commented Aug 21, 2015

@stevengill, I've just submitted a PR with final implementation. See apache/cordova-lib#282. Would appreciate your feedback.
Once this PR landed into master, the next step is to factoring out shared logic into cordova-common.

@nikhilkh
Copy link
Contributor

@nikhilkh nikhilkh commented Oct 20, 2015

Now this API is finalized and implemented. What's a good place to publish docs for this?

@stevengill
Copy link
Contributor

@stevengill stevengill commented Oct 20, 2015

@nikhilkh I would say we should mention it in the readme of the platform repos and somewhere on docs. Docs are just so unorganized right now :(

@nikhilkh
Copy link
Contributor

@nikhilkh nikhilkh commented Oct 20, 2015

Readme for platform repo or cordova-lib is a good start. Eventually, we want to put all of these docs together and publish them on our site.

@stevengill
Copy link
Contributor

@stevengill stevengill commented Oct 20, 2015

Yup. Maybe like a "creating a new platform" section or "working with platforms"

Maybe we need to differentiate docs for "developers working on cordova" + "plugin authors" and docs for people using cordova.

@nikhilkh
Copy link
Contributor

@nikhilkh nikhilkh commented Oct 21, 2015

Yup - creating a new platform is not a common task.

However, integrating a platform in your workflow - gulp/grunt might be relevant for more people. It's un-validated use case and we might have issues with using the API that way. However, in theory these APIs might help with that. On those lines - maybe Api.js should be the index file of the package for the platform.

var android = require('cordova-android')
android.build()

Though for the grunt/gulp story need some more thought- there are other options of using the cordova-lib api itself instead of cordova-android's platform API. There are probably pros and cons for each of these - we should come up with the guidance around this and blog about it eventually.

@stevengill
Copy link
Contributor

@stevengill stevengill commented Oct 21, 2015

+1.

@vladimir-kotikov already set Api.js as main in package.json. So require('cordova-android') should work I believe. Haven't tested that yet.

@vladimir-kotikov
Copy link
Member Author

@vladimir-kotikov vladimir-kotikov commented Oct 21, 2015

require('cordova-android') should work I believe.

Confirm, it works

> var api = require('d:/cordova/cordova-android')
> console.dir(api)
{ [Function: Api] createPlatform: [Function], updatePlatform: [Function] }
@stevengill
Copy link
Contributor

@stevengill stevengill commented Oct 22, 2015

Minor nit pick, can we move this entire proposal in to a sub directory. Right now it would add 5 new files into the root proposals directory. We can also do this after merge though.

@dblotsky
Copy link
Contributor

@dblotsky dblotsky commented Nov 13, 2015

Why isn't this PR in the repo?

@dblotsky
Copy link
Contributor

@dblotsky dblotsky commented Nov 13, 2015

@vladimir-kotikov @stevengill I want to merge this PR so that we can see it in the repo. Please say if you have objections.

stevengill added a commit that referenced this pull request Nov 13, 2015
@stevengill stevengill merged commit 1c07f49 into apache:master Nov 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants