Breaking changes in cordova-create

[raphinesse]

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 ([cordova-create] Simplify interface of main export (§1) #99)
• §2 [RESOLVED] Drop support for reading configuration from <dir>/.cordova/config.json
• §3 New logic for setting attributes in package.json & config.xml ([cordova-create] New logic for setting attributes in package.json & config.xml (§3) #100)
• §4 Do not subscribe CordovaLogger to Cordova events ([cordova-create] Fix event delegation (§4) #101)
• §5 Do not copy "missing" files from default template ([cordova-create] Do not copy "missing" files from default template (§5) #102)
• §6 [RESOLVED] Fetch templates to system temp dir
• §7 Reduce supported template layouts ([cordova-create] Reduce supported template layouts (§7) #103)
• §8 Drop support for linking ([cordova-create] Drop support for linking (§8) #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

Resolved in apache/cordova-create#18

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:

1. Throw an error (my preference)
2. 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:

• www
• hooks
• config.xml

Pain Points

• hooks dir has been deprecated for a long time and is created w/ every new cordova app
• package.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.xml
• package.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:

1. TEMPLATE contains a proper template as documented in the template docs
   cordovaCreate will copy all contents of TEMPLATE/template_src to dir (simplified)
2. A "bare" folder w/out a main module
   cordovaCreate will copy all contents of TEMPLATE except for some blacklisted² files to dir
3. 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! 😉

[dpogue]

A few quick thoughts from skimming this, will try to give it a full read-through over the weekend.

§2 Drop support for reading configuration from <dir>/.cordova/config.json

I thought we had deprecated the .cordova directory a while back, but I still see a bunch of references to it in various bits of code (HooksRunner in cordova-lib being one example).

IMO we should completely remove any special handling for a .cordova folder in the project.

§4 Do not subscribe CordovaLogger to Cordova events

IMO if this is a module without a CLI executable, it should not subscribe CordovaLogger to the event stream. The event stream is meant so that loggers at the higher-level tools (like cordova-cli, or VS Taco) can receive logging output and handle it however they see fit.

§8 Drop support for linking

I've never used this feature, but I do see some value in it when developing templates. If it's not causing a maintenance burden, I'd slightly prefer to keep it around.

We similarly have a --link option when installing platforms and plugins to allow easier local development and testing within a project.

[raphinesse]

@dpogue Regarding §8

I have used the --link option for plugins and it is indeed useful. But that linked the whole plugin folder IIRC. The linking option of cordova-create only links a few items from the template. So if I would add a res folder next to www, it would not show up in the target.

Regarding maintenance: For its apparent use it's too much code IMO. With §7 it would be better and an implementation that just links the whole template would be even nicer of course.

[raphinesse]

It seems it was planned to remove --link-to before #49 (comment). Support was then removed in apache/cordova-lib#456 and re-added in apache/cordova-create@79cace5 addressing https://issues.apache.org/jira/browse/CB-11623 which is still open because it apparently still not satisfies the reporter's requirements.

The reporter's requirements are to create a cordova project with www linking to some existing web app. This could be easily achieved by running

$ cordova create bin com.example.domain APPNAME
$ rm -r bin/www && ln -sr www bin/www

instead of

$ cordova create bin com.example.domain APPNAME --link-to=www

[raphinesse]

Right now, §7 and §8 are major impediments to a sane implementation of #69. Even if we cannot reduce the existing functionality as much as I would want to, I would really appreciate some kind of consensus on these items before continuing with apache/cordova-create#8.

Especially the need for renaming to .gitingore in cordova-create, combined with the linking feature (which I still would wanted to hear valid use cases for) are heavily conflicting.

[brody4hire]

Right now, §7 and §8 are major impediments to a sane implementation of #69.

Agreed on my part.

Especially the need for renaming to .gitingore in cordova-create, combined with the linking feature (which I still would wanted to hear valid use cases for) are heavily conflicting.

Agreed, yes

[dpogue]

Right now, §7 and §8 are major impediments to a sane implementation of #69. Even if we cannot reduce the existing functionality as much as I would want to, I would really appreciate some kind of consensus on these items before continuing with apache/cordova-create#8.

If we're doing this as a major version bump (which we would be for dropping node 4 support), then I'm in favour of saying we only support one template layout. We should also require template to have config.xml at the root instead of under www.

[raphinesse]

I just updated §4. Should I make this a PR document to make it easier to track changes?

[brody4hire]

I just updated §4. Should I make this a PR document to make it easier to track changes?

+1 on my part

[raphinesse]

Closed in favor of separate proposals