Implementation for Unified platform API #235
Conversation
| var configParser = new ConfigParser(configSource || this.getConfigXml()); | ||
| return Q.when(this.parser.update_project(configParser)); | ||
| } | ||
| return Q(); |
There was a problem hiding this comment.
Should we throw in this case? If there is no parser and no overriding implementation, ti's probably a bug.
There was a problem hiding this comment.
Well, in some cases platform may want to do nothing in updateProject. Example is a browser platform, which needs to update config.xml and www content only during prepare.
|
Looks great! The need to duplicate common code like ConfgiParser in cordova-android is a bit annoying and is bound to generate some versioning confusion. But probably it should be dealt with later. One option could be to pass modules like ConfigParser to coraodva-android/Api.js from code that requires Api.js (usually cordova-lib). It's weird and unintuitive, but would solve the code duplication project. |
|
@kamrik, thanks for feedback. Dependency injection of cordova logic to platform was our very first idea, but in addition to limitations, you mentioned, it seems that it also will be impossible to use PlatformAPI without CLI in this case. Another approach, we've been thinking about - factoring out common classes and utilities, like ConfigHelper, superspawn, Requirements-related stuff, etc., to separate module, which then can be used by platforms and cordova-lib. This approach is quite similar to @TimBarham's PR for cordova-serve: #238 What do you think about this? |
|
I don't have strong opinion here, but a bit more inclined towards dependency injection. For some modules like ConfigParser having different versions imported by different platforms can get very frustrating. Even when cordova is used without thc CLI, large parts of cordova-lib would still be needed, whether required()'d directly by the user or implicitly via cordova-android. So as an extreme option we can dependency-inject the entire cordova-lib (properly exposing the relevant classes). Somewhat more elegantly, we could construct a module inside cordova-lib with just the needed classes and inject it when instantiating cordova-android API. It might be useful to later split out this module as a separate npm package (or several packages), but for time being they could still stay part of the lib. Superspawn is definitely a candidate for being a totally separate module. But modules like PluginInfo and ConfigParser are very unlikely to be used separately from each other. Maybe in the long run they should live in some new package named like "cordova-core", or we could move the CLI specific stuff out of cordova-lib and into cordova-cli, then the lib would become that core. That said, I'm fine with the approach you suggested. |
| }; | ||
|
|
||
| // getPlatformApi() should be the only method of instantiating the | ||
| // PlatformProject classes for now. |
There was a problem hiding this comment.
:nit PlatformProject -> PlatformApi ?
|
It will help to generate API docs for the public API surface now. Reviewing the implementation guts and the API in code is not easy. In any case, the platform API might need to be documented for anyone looking to create a platform. Might as well do it now. For that, using jsdoc or other tools to generate docs for the API from code would be super useful. |
|
Overall, it feels like the API is closely bound to current implementation and order as opposed to be more meaningful and self-descriptive. I know there is a constraint of a lot of legacy baggage to support here. |
| function runShellSilent(callback) { | ||
| var silentstate = shell.config.silent; | ||
| shell.config.silent = true; | ||
| callback(); |
There was a problem hiding this comment.
Should there be a try..finally here?
|
Since you've already ported the android implementation from cordova-lib to cordova-android, shouldn't you delete the android parser file from the metadata directory ? As long as it exists, BasePlatformApi.update_www (and potentially other functions) refers to it instead of the intended new code. |
|
If you're instantiating |
| var defaultConfig = path.join(this.root, 'cordova', 'defaults.xml'); | ||
| var ownConfig = this.getConfigXml(); | ||
| // If defaults.xml is present, overwrite platform config.xml with it | ||
| if (fs.existsSync(defaultConfig)) { |
There was a problem hiding this comment.
The old version of the code also handled the case when both defaults.xml and project_dir/config.xml were not present.
In that case, it would grab 'www/config.xml' and copy it to project_dir/config.xml.
Any reason that case isn't handled anymore ?
|
Ah, I see. (ref: android_parser & backward compatibility). However, in case a platform is implemented via the new way, shouldn't we be actually using that ? currently, we're still using the old code, which means the new one never runs. On top of that, if someone wanted to fix or debug an issue, he'd do it on the new code instead of the old one, which is confusing. I suggest, we always prefer the new API when available, and only fall back to old one when the new API isn't available. |
|
|
||
| // parser === null is the special case which means that we've tried | ||
| // to get embedded platform parser and failed. In this case instead of | ||
| // parser's methods will be called PlatformApi default implementations. |
There was a problem hiding this comment.
this sentence is a little confusing.
Maybe reword it to 'In this case, the default behavior as implemented by PlatformApi will be used' ?
|
Closing this since the PlatformApi needs for more detailed design. The discussion is moved to apache/cordova-discuss#9 |
An mplementation of Unified Platform API for cordova, that inspired by this thread in cordova-dev mailing list and this proposal
This PR is the updated version of #226