feat: allow plugins to use the userConfig object to set arbitrary data

[philnash]

Plugins can read this.userConfig but they can't use it to set other data. I am writing a plugin that I think would benefit from a central config store and writing data to this.userConfig would be ideal.

I am proposing that we add a plugins property to the object and allow for plugins to add arbitrary data under their own namespace. e.g.

this.userConfig.setPluginData('my-new-plugin', { this: 'is', really: 'cool' });

this.userConfig.getPluginData('my-new-plugin').really;
// => 'cool';

Checklist

• I acknowledge that all my contributions will be made under the project's license
• I have made a material change to the repo (functionality, testing, spelling, grammar)
• I have read the Contribution Guidelines and my PR follows them
• I have titled the PR appropriately
• I have updated my branch with the main branch
• I have added tests that prove my fix is effective or that my feature works

If you have questions, please file a support ticket, or create a GitHub Issue in this repository.

[philnash]

Just to note, this is just an idea. As a plugin developer I can also use this.config.configDir and add my own config files in there. That may be preferable. Let me know what you think!

[thinkingserious]

With respect to your comment about using this.config.configDir, what prompted you to seek this alternative?

[thinkingserious]

Overall, I like this idea and the clean naming convention.

With this setup, it seems like we need to be careful with name spacing to avoid unintended data manipulation. Perhaps the pluginName needs to be <org>/<name of plugin>.

[philnash]

There's not much we can do about data manipulation, any plugin can overwrite the entire userConfig if it wanted to. The idea that setting the plugin name (which could be defined as "serverless" or "@twilio-labs/plugin-serverless", for example) should be a unique namespace.

A better alternative would be to have the function setPluginData(data) and the name get read from the plugin itself. But I don't know if plugin name/namespace is available to the plugin during runtime. Do you know if that is possible?

[philnash]

With respect to your comment about using this.config.configDir, what prompted you to seek this alternative?

It felt like a good idea to make a conventional space for all plugins to use if they want to rather than to potentially clog up ~/.twilio-cli/ with a bunch of JSON files. Also, this.userConfig is read by the Twilio CLI every time regardless, so if a plugin used it for data then it would already be present by the time the plugin's command is called.

Arguments against are that a plugin could enter a lot of data into userConfig and that could slow down loading of all commands. In that situation it would be better for that plugin to have their own data store (or consider how much data it needs to store).

It's also possible that most plugins don't need this store available, so this would be a waste inside the core. In the plugin I am writing right now, I've implemented my own config store so that I could continue development. I'm ok with this, but I personally think it would be good to have a convention built into the core.

[childish-sambino]

I like the idea of giving each plugin its own space with .twilio-cli/. We have prior art for this in the way we allow for dynamic installation of modules (see requireInstall()).

There's also a getCommandPlugin() method which takes in the command instance and returns details about the plugin the command is from. This could be used to simplify the interface.

I also agree about a single config file getting too bloated and slowing things down. If we combine the two things above, we could load the plugin's config as-needed at runtime from its own space, or any other arbitrary files in that space.

[philnash]

I like the idea of giving plugin authors a way to store config that is simple and controlled by the main CLI.

Are you thinking something like creating a pluginConfig getter and setter on the command object so that a plugin author could fetch it with:

const pluginConfig = this.pluginConfig;
// update pluginConfig
this.pluginConfig = newPluginConfig;

Could add this in BaseCommand, something like:

class BaseCommand extends Command {
  // constructor etc

  get pluginConfig() {
    const pluginName = getCommandPlugin(this).pjson.name;
    const configPath = path.join(this.config.configDir, `${pluginName}.json`);
    try {
      const config = fs.readFileSync(configPath, { encoding: 'utf-8' });
      return JSON.parse(config);
    } catch(error) {
      return {};
    }
  }

  set pluginConfig(config) {
    const pluginName = getCommandPlugin(this).pjson.name;
    const configPath = path.join(this.config.configDir, `${pluginName}.json`);
    fs.writeFile(configPath, JSON.stringify(config);
  }
}

What do you think?

[childish-sambino]

I like that. Also prefer to wrap all this in a separate tree:

path.join(this.config.configDir, 'plugins', pluginName, 'config.json');

Then plugins get their own folder to stick anything else in (like OpenAPI docs ...).

[philnash]

I like it. Will get an update to this PR prepped!

[philnash]

Ok, here's an update based on the above conversation. I am missing tests for the base-command, because I did not have time to work out mocking a plugin in the tests before Friday came to a close. Any hints on that are welcome.

[childish-sambino]

@philnash I've not forgotten about this, but may not have time to review until late next week.

[childish-sambino]

Prefer the tests not do any fs.* operations as it's an implementation detail. Better to set the config twice and verify only the second set of data is returned in the get,

[childish-sambino]

Same here. Should just set the config and then get it (with a new instance of PluginConfig to verify it's not just cached in memory).

[childish-sambino]

The APIs here differ from what's done for the main config file: writeFileSync instead of writeJSON and readFileSync instead of readJSON. Should be made consistent.

[childish-sambino]

nit: this block is duplicated (only a nit since it doesn't violate the rule of 3)

[philnash]

Updated the read/write functions, which meant I updated to async functions.
Removed the fs methods from tests, which left two of the tests doing the same, so removed the unnecessary test.

[childish-sambino]

👍

[childish-sambino]

nit: not required

[philnash]

Sorted!

[philnash]

Do we reckon we can get this into the next CLI release?

I'd like to use this in my new Assets plugin (see WIP PR here) and I can pull out this configStore implementation and replace it with this (with a warning to upgrade the CLI if these methods aren't available).

[childish-sambino]

Yup, it will be included in the next release.