Allow functions

[liron-navon]

This is a feature request, can we allow the support of functions? I know this have been raised in the past, and the solution was gettered, but here is a getter:

 get sayHello() {
            console.log('saying hello...');
            return `echo "Done"`
 }

And the getter is ending up being called three times.

➜  nps sayHello
saying hello...
saying hello...
saying hello...
nps is executing `sayHello` : echo "Done"
Done

Also maybe supporting functions that return a string, or even functions that return a promise that resolves to a string for asynchronous code - could be great for different use cases.

USE CASE:

I have a project that needs to run some commands and update a file when the command is called, so I want to be able to use fs when calling the script.

[kevjin]

Hey Liron, I couldn't reproduce your issue with the sayHello getter being called three times. Could you show how it's imported to package-scripts?

[kevjin]

For your second part regarding functions that return strings, especially async, it's a feature that we've discussed in the past and don't want to implement to avoid adding further complexity to nps. The example with fs can be referred to in #184. Sorry, but thanks for the issue!

[liron-navon]

@kevjin Thank you for your time.

scripts.js

module.exports = {
  scripts: {
     get sayHello() {
            console.log('saying hello...');
            return `echo "Done"`
     }
  }
}

and calling it with nps v5.9.3 installed globally:

nps sayHello

[liron-navon]

@kevjin Made a PR, it's kinda big but its 90% tests, can you please review it? do I need to add anyone specific as a reviewer here?

https://github.com/kentcdodds/nps/pull/197

[liron-navon]

Too bad, please close this issue.

[kevjin]

Hey @liron-navon, I saw the PR you made and appreciate your enthusiasm for supporting this feature and nps. We closed most of the issues you referenced in your PR because supporting functions is something we explicitly stated we do not want to do in our roadmap, for the reason of complexity as I mentioned above. I think @kentcdodds would agree with me regarding this issue and PR. Sorry about this, but again, I really appreciate you taking your time to write this. Thanks!

[Venryx]

Hey Liron, I couldn't reproduce your issue with the sayHello getter being called three times. Could you show how it's imported to package-scripts?

I found out why the triple-log issue showed up for Liron but not others: the issue only shows up when the getter is "nested" inside a group.

So this has the issue: (logs saying hello three times)

exports.scripts = {
  group: {
    get sayHello() {
      console.log('saying hello...');
      return `echo "Done"`
    }
  }
}

// in terminal
nps group.sayHello

But this is fine: (logs saying hello only once)

exports.scripts = {
   get sayHello() {
      console.log('saying hello...');
      return `echo "Done"`
   }
}

// in terminal
nps sayHello

[Venryx]

I found out why the triple-log is occurring by the way.

The reason is that the scripts.sayHello entry is having a "copy" of the entry added under the key say-hello, from this line: https://github.com/sezna/nps/blob/57989a24ff6876b3d5245f7e00b76aaf39296d31/src/get-script-to-run.js#L9

Now that exports.scripts has two keys (sayHello and say-hello) with the same accessor/getter function, it causes that getter to run twice once this line is hit: https://github.com/sezna/nps/blob/57989a24ff6876b3d5245f7e00b76aaf39296d31/src/get-script-to-run.js#L12

(The lodash cloneDeep function iterates through the config/scripts object, thus triggering both copies of the accessor/getter.)

[Venryx]

@kevjin By the way, you might be happy to hear that I found a way to achieve your feature request by adding some monkey-patching within the package-scripts.js file. It's kinda hacky, but it works, and doesn't take that much boilerplate:

const Dynamic = commandStrGetter=>{
    const result = new String("[placeholder for dynamically-evaluated command-string]");
    result.commandStrGetter = commandStrGetter;
    return result;
};
const join_orig = Array.prototype.join;
Array.prototype.join = function(...args) {
    // Here is the relevant location in the nps source code: https://github.com/sezna/nps/blob/57989a24ff6876b3d5245f7e00b76aaf39296d31/src/index.js#L59

    // If we're concatenating the script-entry with its args (just before execution)...
    //  ...and we find a String object produced by the Dynamic function above (rather than a primitive string like normal)...
    //  ...then intercept and replace the String object with the result of its commandStrGetter().
    if (this[0] instanceof String && this[0].commandStrGetter != null) {
        this[0] = this[0].commandStrGetter();
        //this.length = 1; // optionally, chop off any arguments (ie. if you want arguments handled by package-scripts.js rather than the command it launches)
    }
    return join_orig.apply(this, args);
};

exports.scripts = {
    sayHello: Dynamic(()=>{
        console.log('saying hello...');
        return `echo "Done"`;
    }),
    otherScript: "echo otherScriptDone",
};

With the package-scripts.js above, here are the results: (confirmed just now in fresh project)

> nps sayHello

saying hello...
nps is executing `sayHello` : echo "Done"
"Done"

> nps otherScript

nps is executing `otherScript` : echo otherScriptDone
otherScriptDone

Note that saying hello... is logged for the first case, but not for the second. 🙂

---

Also, while the above does not support an asynchronous (ie. promise-based) return-value for the commandStrGetter function, you can achieve a similar thing by having the commandStrGetter return an empty command, and then just call the command you wanted manually at the end of that function.

Example:

const fs = require('fs').promises;
const child_process = require('child_process');
const exec_promise = require('util').promisify(child_process.exec);

const Dynamic_Async = asyncCommandRunnerFunc=>{
    return Dynamic(()=>{
        asyncCommandRunnerFunc();
        // just return an empty command
    });
};

exports.scripts = {
    sayHello: Dynamic_Async(async()=>{
        await fs.writeFile('filename.txt', 'test');
        await exec_promise("whatever command you wanted at the end");
        console.log("Done");
    }),
    otherScript: "echo otherScriptDone",
};

This does "remove most of the usefulness of nps" for that particular entry (ie. nps no longer "does anything" other than map a command-name to a command-string-getter function), but the point is that it lets you "selectively override" nps's default behavior, only doing so when you need that extra flexibility.