Recently I've done quite a lot of cleanup work in cordova-create (see apache/cordova-create/pull/13). This PR includes only non-breaking changes. However, IMHO there is still a lot of technical debt that can only be addressed by making some breaking changes.
For most of these changes I already have some code in my local repo, so all of this could probably be implemented in short to no time. I'd just have to get it done before work hits again.
In what follows, let cordovaCreate be the anonymous function that is the main export of the cordova-create package.
TOC
- §1 Simplify arguments accepted by
cordovaCreate (#99)
- §2 [RESOLVED] Drop support for reading configuration from
<dir>/.cordova/config.json
- §3 New logic for setting attributes in package.json & config.xml (#100)
- §4 Do not subscribe CordovaLogger to Cordova events (#101)
- §5 Do not copy "missing" files from default template (#102)
- §6 [RESOLVED] Fetch templates to system temp dir
- §7 Reduce supported template layouts (#103)
- §8 Drop support for linking (#104)
§1 Simplify arguments accepted by cordovaCreate
Current situation
Arguments
/**
* Usage:
* @dir - directory where the project will be created. Required.
* @optionalId - app id. Required (but be "undefined")
* @optionalName - app name. Required (but can be "undefined").
* @cfg - extra config to be saved in .cordova/config.json Required (but can be "{}").
* @extEvents - An EventEmitter instance that will be used for logging purposes. Required (but can be "undefined").
**/
// Returns a promise.
function (dir, optionalId, optionalName, cfg, extEvents) {...}Properties of cfg used in cordovaCreate¹
{
lib: {
www: {
// The path/url/npm-name to the template that should be used
url: String,
// Symlink instead of copy files from template to dir
link: Boolean,
// Template is only fetched when true.
// Template files are only copied when true.
// If false, only some "mandatory" files are copied over from
// `cordova-app-hello-world`
template: Boolean,
// Deprecated alias for url (w/out deprecation warning)
uri: String
}
}
}¹: neither the cfg object nor parts larger than single leaf properties are
passed outside the scope of this module, so a local view on this is all we need.
Pain Points
- Required but optional™ arguments
- Deeply nested structure of configuration
- Confusing naming and unclear semantics of configuration
Proposal
Arguments
/**
* Creates a new cordova project in dest.
*
* @param {string} dest - directory where the project will be created.
* @param {Object} [opts={}] - options to be used for creating a new cordova project.
* @returns {Promise} Promise that resolves when project creation has finished
*/
function (dest, opts) {...}Structure of opts
{
// Attributes to be set in package.json & config.xml
id: String,
name: String,
version: String,
// The path/url/npm-name to the template that should be used
template: String,
// Symlink instead of copy files from template to dest
link: Boolean,
// An EventEmitter instance that will be used for logging purposes
// If not dropped as proposed in §4
extEvents: EventEmitter
}§2 Drop support for reading configuration from <dir>/.cordova/config.json
Current situation
Before doing anything interesting, cordovaCreate looks for the file
<dir>/.cordova/config.json. If it exists, its contents are parsed as JSON and
merged with cfg.
Pain Points
- Undocumented feature (AFAIK; related phonegap issue)
- Completely untested feature (at least in
cordova-create)
- Unclear use cases (As config can be passed to CLI as JSON too)
- Makes it harder to reason about code
Proposal
Drop it like it's hot!
§3 New logic for setting attributes in package.json & config.xml
I'm talking about the logic behind setting the appropriate fields for App ID,
App Name and App Version in the files package.json and config.xml.
Current situation
The most common strategy is to set the attribute in both files iff a value for
it was given to cordovaCreate. Exceptions are
- App ID in
package.json: set attribute if value given else set to helloworld
- App Version in both files: always set to
1.0.0
Pain Points
- Strategies employed for setting the attributes differ
- Falling back to hard coded defaults is not very flexible (e.g. I usually start my version numbering at
0.1.0)
Proposal
For every file f and every attribute attr, do the following
if (attr in opts) {
// Save attribute value passed to cordovaCreate to f
f[attr] = opts[attr]
} else if (attr in f) {
// Attribute already present in f and no override specified
// => Leave the existing value untouched
} else if (isRequired(attr, f)) {
handleMissingRequiredAttribute(attr, f)
}where isRequired would have to be defined adequately
and handleMissingRequiredAttribute could either:
- Throw an error (my preference)
- Save some hard coded fallback value to
f (possibly warn that f is invalid)
Related issues
CB-12274 - widget version number not copied from template config.xml file
§4 Do not subscribe CordovaLogger to Cordova events
Current situation
Right at the start, cordovaCreate calls the following function with the
extEvents argument passed to it.
/**
* Sets up to forward events to another instance, or log console.
* This will make the create internal events visible outside
* @param {EventEmitter} externalEventEmitter An EventEmitter instance that will be used for
* logging purposes. If no EventEmitter provided, all events will be logged to console
* @return {EventEmitter}
*/
function setupEvents (externalEventEmitter) {
if (externalEventEmitter) {
// This will make the platform internal events visible outside
events.forwardEventsTo(externalEventEmitter);
// There is no logger if external emitter is not present,
// so attach a console logger
} else {
CordovaLogger.subscribe(events);
}
return events;
}Pain Points
- Every call to
cordovaCreate w/out extEvents subscribes CordovaLogger to
events again w/ no possibility to unsubscribe it.
cordovaCreate is tightly coupled to the Cordova event bus singleton.
Proposal
I don't know much about the established patterns to use the Cordova event bus,
but the following would make sense to me as replacements for setupEvents in cordovaCreate:
1. Send events to cordova-common's events
if (extEvents) {
events.forwardEventsTo(extEvents);
}Forwarding events makes no sense in this case. If we emit events to a global event bus, we don't need to provide the service to setup event forwarding for callers. The callers could just call events.forwardEventsTo(extEvents) themselves. The forwarding we setup isn't scoped to cordovaCreate either.
2. Only emit events if an EventEmitter was passed to cordovaCreate
const emit = extEvents
? (...args) => extEvents.emit(...args)
: () => {};This would break the coupling to the global event bus and enable callers to subscribe to cordovaCreate events only.
§5 Update required template files/dirs
Some files and dirs are copied to dest from cordova-app-hello-world if they
are not present in the used template.
Current situation
These files and dirs are:
Pain Points
hooks dir has been deprecated for a long time and is created w/ every new cordova apppackage.json could initially be missing from an app
Proposal
A: Remove this behavior (Preferred)
- Make
cordovaCreate more agnostic to the template layout
- Principle of least surprise for template authors
B: Update this behavior
Update required files and dirs to:
www (not even sure if this is necessary)config.xmlpackage.json
Related
#78
§6 Fetch templates to system temp dir
Current situation
cordovaCreate uses cordova-fetch to save any remote templates to
$HOME/.cordova before copying the assets to the created app.
Pain Points
- Litters the user's home directory (How I hate this...)
- Fetched template is not deleted after usage
Proposal
Fetch templates to system temp dir w/ cleanup on process exit (provided by tmp package).
This functionality might even be provided by cordova-fetch itself.
I don't even think this one would be a breaking change, but I'm not sure so it is included here.
§7 Reduce supported template layouts
Current situation
cordovaCreate supports handling of at least three different layouts for the
templates that are referenced by url. Let TEMPLATE be the directory on disk that
contains the resolved template. Then there are these three cases:
TEMPLATE contains a proper template as documented in the template docs
cordovaCreate will copy all contents of TEMPLATE/template_src to dir (simplified)- A "bare" folder w/out a main module
cordovaCreate will copy all contents of TEMPLATE except for some blacklisted² files to dir
basename(TEMPLATE) === 'www'
cordovaCreate will copy TEMPLATE to dir/www
Additionally cordovaCreate still handles (and corrects) the case that
config.xml is located in TEMPLATE/www instead of TEMPLATE.
²: package.json, RELEASENOTES.md, .git, NOTICE, LICENSE, COPYRIGHT, .npmignore
Pain Points
- AFAICT, only case 1 is documented
- Increased code & documentation complexity
Proposal
- Drop support for 3.
- Document or drop 2.
- Drop Support for templates with
www/config.xml.
§8 Drop support for linking
I'm not so sure about this one, but after all this is called cordova-discuss, right?
Current situation
Ignoring all special cases of §7, cordovaCreate w/ enabled link option will
- symlink folders
www, merges and hooks
- symlink the file
config.xml
Pain Points
- I don't get what use cases there could be for this.
Even if there are some, this does not seem like something that is used often.
Is there any telemetry data on this?
hooks is deprecated- Needs special privileges on Windows since it also links files.
Proposal
Drop it.
Conclusion
Let me break all the things! 😉
React
Typescript
Django
Laravel
Facebook
Microsoft
Google
Alibaba
D3
Tencent