Refactor Adapter Hook Methods about ember-metrics HOT 6 CLOSED

After spiking a bit I've settled on install and uninstall. I don't believe I can do an easy deprecation, so I'm simply going to write an upgrade guide. I'm going to fix one small issue and then do a release some time this week.

from ember-metrics.

@GreatWizard @jfdnc pinging the both of you as your minds have been in this place for a little bit. I'd be curious to hear what you think. Something I'm still unsure about is how this design may impact moving towards an Embroider safe future. It might be really cool to do dynamic imports instead relying upon Ember's current mechanism. I'm all ears here.

from ember-metrics.

@jherdman I think both of these proposals have some value for sure. Having an API function that returns HTMLScriptElement lets us handle teardown without the consumer having to do additional setup, and still exposing cleanup could still be valuable as well.

For the potential setup functionality, I think we can provide configurability and also minimize necessary API by just using constructor in the base adapter

// as POJO
export default class BaseAdatper {
  constructor(config) {
    this.config = config;
    const scripts = this.injectScripts();
    // register `data-*` attrs for teardown
    this._setupEmberMetricsScripts(scripts);
  }
  cleanup() {
    // script teardown
  }
}

And then in an extended adapter

export default class MyAdapter {
  constructor() { // optional if setup other than `injectScripts` call desired
    super(...arguments); 
    // additional setup
  }
  injectScripts() {
    // config key assertions
    // create script(s) and append to dom
    return scripts;
  }
  cleanup() { // optional if teardown other than script removal desired
    super.cleanup(...arguments);
    // additional cleanup
  }

I know that pattern doesn't immediately help the deprecation path away from init, but I think adding an explicit configure method eventually won't be necessary given that setup can be done in the constructor.

from ember-metrics.

As for the Embroider bit of this, I'm actually spending some time this week diving more into Embroider and what that looks like for addon compatibility. Will share an opinion on that if there is something relevant for the scope of this addon.

from ember-metrics.

Any objections to making the injectScripts method also optional? Some adapters will use an npm package instead of a script tag so in that case the hook isn't strictly needed.

I also like the idea of using the constructor as @jfdnc described 👍

from ember-metrics.

Any objections to making the injectScripts method also optional?

None at all. I may make these methods static as well. It seems like the kind of thing that an instance needn't concerned about.

from ember-metrics.