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

Simplify interface of main export (§1) #21

Closed
raphinesse opened this issue Sep 10, 2018 · 2 comments · Fixed by #40
Closed

Simplify interface of main export (§1) #21

raphinesse opened this issue Sep 10, 2018 · 2 comments · Fixed by #40
Assignees

Comments

@raphinesse
Copy link
Contributor

raphinesse commented Sep 10, 2018

From @raphinesse on July 5, 2018 8:23

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 apache/cordova-discuss#89

Copied from original issue: apache/cordova-discuss#99

@raphinesse
Copy link
Contributor Author

From @brodybits on July 5, 2018 14:29

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
Contributor 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 raphinesse changed the title [cordova-create] Simplify interface of main export (§1) Simplify interface of main export (§1) Sep 10, 2018
@raphinesse raphinesse self-assigned this Sep 10, 2018
@raphinesse raphinesse assigned raphinesse and unassigned raphinesse Sep 10, 2018
raphinesse added a commit to raphinesse/cordova-create that referenced this issue Oct 15, 2019
raphinesse added a commit to raphinesse/cordova-create that referenced this issue Oct 16, 2019
raphinesse added a commit to raphinesse/cordova-create that referenced this issue Oct 16, 2019
@erisu erisu closed this as completed in #40 Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant