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-create] Simplify interface of main export (§1) #99

Closed
raphinesse opened this issue Jul 5, 2018 · 3 comments
Closed

[cordova-create] Simplify interface of main export (§1) #99

raphinesse opened this issue Jul 5, 2018 · 3 comments

Comments

@raphinesse
Copy link

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
}

Migrated from #89

@brodybits
Copy link

I do not see if the public cordovaCreate function API, which seems to be exported by both cordova-create and cordova-lib, was documented anywhere other than in a README. From NPM I see very few direct dependents of cordova-create, more dependents of cordova-lib including some gulp plugins (which seem to be a bit old).

I think it would be good to get the Cordova tooling API documented. Should this be raised as a separate task?

If we change the public cordovaCreate function API on cordova-create I would favor allowing the caller to inject the fetch function for smoother mocking. (No need for cordova-lib to export this option, I think.)

@raphinesse
Copy link
Author

Thanks for your feedback @brodybits !

I think it would be good to get the Cordova tooling API documented. Should this be raised as a separate task?

Yes, the tooling libs definitely need better documentation. Prefereable in their respective README.
I already added that point to our release planning document, so I think we are fine for now.

If we change the public cordovaCreate function API on cordova-create I would favor allowing the caller to inject the fetch function for smoother mocking. (No need for cordova-lib to export this option, I think.)

I'm not too fond of polluting the public interface for testing only. Jasmine spies and rewire work pretty well for me, given the test subject has a reasonable structure.

If we need DI for real-world use cases, I'm open to it. I would probably move it to a separate issue though, as it is an additional feature.

@raphinesse
Copy link
Author

This issue was moved to apache/cordova-create#21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants