RFC for "Inline Plugins"

[achambers]

RENDERED

This is a continuation from achambers#1 which was opened against the wrong repo.

[lukemelia]

@achambers perhaps the RFC should include an example of how one would extend from the base plugin class when defining an inline plugin. I don't think the second section (overriding hooks for existing plugins) is clear -- is this the same as subclassing an existing plugin? Could one do that by importing it, extending it and including it in inline plugins?

[achambers]

@lukemelia said:

@achambers perhaps the RFC should include an example of how one would extend from the base plugin class when defining an inline plugin.

I was trying to give the simplest case, which I imagine is what most people will want this for. After all, the base plugin just returns the same object with some extra functionality. Having said that, I'd say most people probably just used the base plugin for the most part so that might be most familiar.

I'll add that example in as well, to show that, just as with creating stand alone plugins, you can use the base plugin too.

I don't think the second section (overriding hooks for existing plugins) is clear -- is this the same as subclassing an existing plugin? Could one do that by importing it, extending it and including it in inline plugins?

Great question. So, my line of thought there as that we would be exposing a way to override hooks that will be added to the plugin instance, rather than override the plugin itself.

Love to explore your thinking here. Could you elaborate on a use case on where you might want something more than simply overriding/adding hooks to the existing plugin instance?

[achambers]

@lukemelia Just re-reading your comment again and noticed I misinterpreted your questions slightly.

is this the same as subclassing an existing plugin?

The second part is not the same as subclassing a plugin. The intention of this section is to describe the ability to modify existing plugin instances by implement new hooks or overriding existing hooks.

Could one do that by importing it, extending it and including it in inline plugins?

Technically, yeh, I suppose they could. I guess they would need to do it something like this:

let RedisPlugin = require('ember-cli-deploy-redis');

// ...snip...

inlinePlugins: [
  {
    name: 'log-to-console',

    createDeployPlugin(/*options*/) {
      let pluginInstance = RedisPlugin.createDeployPlugin(...arguments);
    
      let pluginInstance.didBuild = function(context) {
        console.log(`Project files built in: ${context.distDir}`);
      };

      return pluginInstance;
    }
  }
]

Is this sort of what you were referring to? It feels kind of ugly but I'm not sure of a better way to do it because, as it stands, I'm proposing that the objects in the inlinePlugins array essentially be the ember-cli addon object (as opposed to the plugin instance object that's returned from the createDeployPlugin function. To extend the hooks you need to modify the object returned by createDeployPlugin. I'm hesitant to make the objects in the inlinePlugins array be the plugin instances themselves because that is bypassing parts of the pipeline that are responsible for aliasing and ordering plugins etc. The intention was to hook in as high up the pipeline as possible and allow a user to, essentially, add the inlinePlugins property be an alternative source from which to discover ember-cli addons that are ECD plugins.

This has kind of turned in to a bit of a brain dump there as I explored my own thoughts while writing.

Does it make sense? Does it answer your question?

[lukemelia]

It helps to read the thinking here. I think learnability would be best supported by using well-understood extensibility tactics rather than introducing what is essentially a syntax to modify existing instances of objects to add new hooks or overriding existing hooks. i.e. let's either provide the ability to subclass prior to instantiation, OR the ability to access and mutate instances after instantiation, but not a novel way of extending a plugin.

The point about inlinePlugins being a collection of plugin "factories" and not a collection of plugins is an good one and gives me pause about the naming of this property. Is there something better we can use?

[achambers]

OR the ability to access and mutate instances after instantiation, but not a novel way of extending a plugin.

This is interesting. I'd like to explore this. Instead of:

redis: {
  pipeline: {
    hooks: {
      upload() {
      }
    }
  }
}

we could do something like:

redis: {
  pipeline: {
    mutatePluginInstance(pluginInstance) {
      // enhance existing hook
      let oldUpload = pluginInstance.upload;
      pluginInstance.upload = function() {
        // do something

        return oldUpload(...arguments);
      };

      // override existing hook
      pluginInstance.didUpload = function() {
        // do something
      }

      // implement new hook
      pluginInstance.didDeploy = function() {
        // do something
      }
      return pluginInstance;
    }
  }
}

I'm not so much of a fan of the pipeline property here, but it does feel like it'd be good to namespace this somehow, so that it standouts from the regular config props that the plugin supports. Open to suggestions here.

Also, not mad about the naming of mutatePluginInstance but trying to be intention revealing for the example. We can riff on this.

The point about inlinePlugins being a collection of plugin "factories" and not a collection of plugins is an good one and gives me pause about the naming of this property. Is there something better we can use?

I don't mind so much about the naming because that's what we use, although that might be more internally in the code more than publicly. We refer to the factory as plugins everywhere. Then when we alias plugins, we end up with plugin instances. This is no different really. This is an array plugins just like you yarn install plugins.

Is there any difference?

What will be possible here is also this:

pipeline: {
  alias: {
    'log-to-console': { as: ['logA', 'logB'] }
  },

  inlinePlugins: [
    {
      name: 'log-to-console',
      createDeployPlugin(options) {
        name: options.name,

        didBuild(context) {
          console.log(`Project files built in: ${context.distDir}`);
        }
      }
    }
  ]
},

logA: {
  // config here
},

logB: {
  // config here
}

[achambers]

In regards to the naming, I guess the two potential paths we could take are:

1. Plugin > PluginInstance or;
2. PluginFactory > Plugin

Currently, 1 is how we have thought and communicated things to date.

[lukemelia]

I'm +1 on the approach for adding plugins.

I think we should split off/defer the concept of mutating plugins to a separate effort. I don't like any of the solutions we've come up with yet, and I'm not clear on the use case either.

[achambers]

@lukemelia said

I think we should split off/defer the concept of mutating plugins to a separate effort. I don't like any of the solutions we've come up with yet, and I'm not clear on the use case either.

Yep, that was my original intention. I'm in favour of that.

[achambers]

@lukemelia I'm having a little internal debate with myself. Wondering if you could enlighten me with your thoughts.

The pipeline.inlinePlugins array contains an object that is the same shape as the one that is exported in a standalone plugin in an ember-cli addon. eg:

{
  name: 'ember-cli-deploy-build',

  createDeployPlugin(options) {
    return {
      name: options.name,
      willDeploy() {}
    };
  }
}

Currently we have an expectation in our code that the addon name begins with ember-cli-deploy-. (The intentions for this made sense at the time but in hindsight I'm not sure how much it's needed in reality. But that's a different conversation)

When defining an inlinePlugin, it feels redundant to require ember-cli-deploy- at the start of the name which makes me desire to not require that. However, in doing that it then creates a subtle difference between creating a standalone plugin and create an inline one which also feels undesirable.

I think I'm beginning to lean on keeping them symmetrical now, then, as a separate thread, opening up the conversation as to whether the requirement is needed at all, across the board.

Do you have any strong feelings one way or the other?

[duizendnegen]

Currently we have an expectation in our code that the addon name begins with ember-cli-deploy-.

From DeployJS point of view (which is just an abstract layer over Ember CLI Deploy to bring it to React / Vue / Angular), not requiring ember-cli-deploy- meant addressing the loader behaviour, too. See https://github.com/deployjs/deployjs-cli/blob/master/tasks/discover.js - this generally was a task that was offloaded as far as I remember (luckily) to the Ember framework, and now had to be picked up elsewhere.
The tricky part for backward compatibility with this is then the deploy.js config file, because in the pipeline, we chop off ember-cli-deploy- before feeding it to configuration. A file that had

return { "s3": { "setting": "value" } }

doesn't work as before then if you have to specify ember-cli-deploy-s3 instead.

[lukemelia]

@achambers I don't think we need to require ember-cli-deploy- for inline plugins. It was designed originally to guarantee uniqueness by leaning on npm's unique package name requirements, but it's not relevant for inline plugin definitions.

[ghedamat]

@achambers @lukemelia a bit late to the party here but my initial gut reaction seems to be confirmed after reading your discussion

I think "part 1" of this RFC is very clear and good to go (adding plugins) while the second part (editing config) is not so yeah I think we all agreed on splitting 👍

[ghedamat]

re: the naming, agreed with @lukemelia iirc (hopefully I'm right) ember-cli-deploy- was only required because of the way we use for plugin loading but yeah I don't see why we would need it for this

[achambers]

@lukemelia said:

It (ember-cli-deploy name prefix) was designed originally to guarantee uniqueness by leaning on npm's unique package name requirements

@ghedamat said:

ember-cli-deploy- was only required because of the way we use for plugin loading

So, correct me if I'm wrong but the name property of the index file in an addon has nothing to do with npm right? The name in the package.json is what npm uses to determine the name of the package and hence enforce the unique package naming no?

I totally understand our intention initially. I'm just not convinced it actually has anything to do with the name property of the addon. There is nothing forcing a user to name the addon the same thing that the module is named. It's more of a side affect of the addon being named the same thing as the module when it's created.

So, the expectation in our code that the name property is prefixed with ember-cli-deploy-* actually serves no purpose whatsoever as our code simply strips it off when working out the name of the plugin.

I totally agree with ye that the prefix is not needed for inline plugins. In fact I'm proposing that it's not needed for standalone plugins either. However that part is a separate RFC/PR/discussion.

So, my preference (I flip flop here but for now this is it) would be to keep things symmetrical between standalone and inline plugins (for learnability, and consistency - I just see the deploy.js as another source from where to discover plugins), and keep the prefix, for now, and follow up with an across the board change after. This removes the slightly confusing "Well, you need to name them like this when creating a standalone plugin, but like this when you create an inline plugin" situation.

Just to clarify, both of ye (@lukemelia and @ghedamat ) are happy for the naming "requirements" of inline plugins to be different to that of standalone plugins in this case?

[achambers]

We could, when evaluating the inline plugin name, check for the ember-cli-deploy- prefix and if it exists, strip it. This would mean that, if someone tries to use inline plugins like they are used to with stand alone plugins, and adds the prefix, then everything works. Likewise, we can recommend to folks, and give examples in our docs, of not using the prefix.

Then we can also discuss, in a separate thread, whether this same logic could/should be applied for the stand alone addon name.