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

Feature idea: supply a decorator API? #144

Open
chriskrycho opened this issue Jun 23, 2017 · 8 comments
Open

Feature idea: supply a decorator API? #144

chriskrycho opened this issue Jun 23, 2017 · 8 comments

Comments

@chriskrycho
Copy link

chriskrycho commented Jun 23, 2017

This is obviously a bit out before we could land it, but as I was looking at the current (really solid) API it occurred to me that class method decorators (after ES6-style classes land… 🤞) could make for a really nice API:

import { trackEvent } from 'ember-metrics';

class Foo extends Component {
  @service metrics;

  @trackEvent('some-action-yeah')
  someMethodWhichShouldTriggerAnAction() {
    // ...
  }
}

That's obviously a very rough idea, and obviously it's quite a ways before we could land it, but I figured I'd throw it out as something to start thinking through – including even whether it's a good idea – ahead of time.

@allthesignals
Copy link

allthesignals commented Oct 16, 2017

@chriskrycho I've been trying to write a trackEvent decorator, and @pzuraq generously provided an outline on how to define this:

export default function trackEvent(eventCategory, eventAction) {
  return (target, name, desc) => {
    const descriptor = desc;
    const originalValue = descriptor.value;

    descriptor.value = function(...args) {
      this.get('metrics').trackEvent(eventCategory, eventAction);
      originalValue.call(this, ...args);
    };

    return descriptor;
  };
}

This seems like a great candidate for decorators. If ember computed decorators are actively maintained, why not?

Only question I have is how best to handle accessing the metrics service. Maybe there's a way the decorator can just do that.

@pzuraq
Copy link

pzuraq commented Oct 16, 2017

Decorators also have access to the target, so you could add the metrics service injection if it doesn't exist on the target class already.

@allthesignals
Copy link

allthesignals commented Oct 16, 2017

So something like this seems okay:

// utils/track-event.js
import Ember from 'ember';

const { service } = Ember.inject;

export default function trackEvent(eventCategory, eventAction) {
  return (target, name, desc) => {
    const descriptor = desc;
    const originalValue = descriptor.value;

    descriptor.value = function(...args) {
      if (!this.get('metrics')) {
        this.set('metrics', service());
      }

      this.get('metrics').trackEvent(eventCategory, eventAction);

      originalValue.call(this, ...args);
    };

    return descriptor;
  };
}

@pzuraq
Copy link

pzuraq commented Oct 16, 2017

That's alright, but it may be better to do it at class definition time rather than when the action gets called for the first time:

import Ember from 'ember';

const { service } = Ember.inject;

export default function trackEvent(eventCategory, eventAction) {
  return (target, name, desc) => {
    const descriptor = desc;
    const originalValue = descriptor.value;

    if (!('metrics' in target)) {
      target.metrics = service('metrics');
    }

    descriptor.value = function(...args) {
      this.get('metrics').trackEvent(eventCategory, eventAction);

      originalValue.call(this, ...args);
    };

    return descriptor;
  };
}

This way all instances of the class would get the same injection, as well as subclasses. It may also be a good idea to make this a private key (__metrics or something like that) so it doesn't effectively make metrics a reserved property on classes. If it ends up being a duplicate injection it probably won't be super expensive, vs the api benefit of not reserving the key.

@kellyselden
Copy link
Collaborator

This would be an optional ember-metrics-decorators right? I don't know if depending on ember-decorators would be too much baggage for this addon.

@allthesignals
Copy link

allthesignals commented Oct 17, 2017

I wonder if #167 is a better solution? I'm finding I'm mostly using the decorator for actions. Anyway, @kellyselden, I think that probably makes sense - ember-decorators is an addon for computed properties (and other things in the framework), not part of the framework (yet).

Separately, I couldn't get @pzuraq's approach to work (I don't think you can inject services that way, isn't target the function itself?), but I did add some logic that lets you specify an identifying property so an event can be data-driven. I suppose this is where #167 would come in handy - a helper that would pipe actions together and explicitly tag an action with data.

function trackEvent(eventCategory, incAction, incLabel, eventValue) {
  return (target, name, desc) => {
    const descriptor = desc;
    const originalValue = descriptor.value;

    descriptor.value = function(...args) {
      originalValue.call(this, ...args);

      if (!this.get('metrics')) {
        this.set('metrics', service());
      }

      let eventAction = incAction;
      let eventLabel = incLabel;

      // allow getting prop names for values
      if (eventAction) {
        const actionIdentifier = this.get(eventAction);

        if (actionIdentifier !== undefined) {
          eventAction = actionIdentifier;
        }
      }

      if (eventLabel) {
        const labelIdentifier = this.get(eventLabel);

        if (labelIdentifier !== undefined) {
          eventLabel = labelIdentifier;
        }
      }

      this.get('metrics').trackEvent(
        'GoogleAnalytics',
        { eventCategory, eventAction, eventLabel, eventValue },
      );
    };

    return descriptor;
  };
}

@pzuraq
Copy link

pzuraq commented Oct 17, 2017

target is the prototype of the class, if you're using ES classes. If you're using objects then I believe it would be the object itself, but that's deprecated - you won't be able to use decorators on POJOs in the near future.

Looking at the compiled output of the @service decorator, it does look like it gets applied to every instance of the class, so the first approach wouldn't be any worse than what we currently do. From a class perspective it would make sense to have the injection be on the prototype, will have to dig in and see why that isn't working.

@chrismllr
Copy link
Contributor

This is an older thread, but modifiers provide a suuuuper simple way to do something like this, right in the DOM (for anyone who comes upon this thread!)

{{! create button }}
<button {{track-event "Created User" properties=(hash performedBy=this.session.currentUser.username)}}>
  Submit
</button>

{{! input, utilizing "focus" }}
<SearchInput {{track-event "Search" "focus" properties=(hash resource="Vehicles")}}>
import Modifier from 'ember-modifier';
import { inject as service } from '@ember/service';
import { action } from '@ember/object';

export default class TrackEventModifier extends Modifier {
  @service metrics;

  get eventName() {
    // the name of the event to track, first positional argument
    //
    // {{track-event eventName}}
    //               ~~~~~~~~
    //
    return this.args.positional[0];
  }

  get domEvent() {
    // get the optional DOM event name for use on other
    //
    // {{track-event eventName "focus"}}
    //                         ~~~~~~~
    //
    return this.args.positional[1] || 'mouseup';
  }

  get properties() {
    // the `properties` named argument passed to the modifier.
    // Allows passing custom event properties to the analytics tools
    //
    // {{track-event eventName properties=(hash prop1=prop1)}}
    //                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    //
    return this.args.named.properties;
  }

  @action
  onEventTriggered() {
    this.metrics.trackEvent({
      event: this.eventName,
      ...(this.properties || {})
    });
  }

  didInstall() {
    this.element.addEventListener(this.domEvent, this.onEventTriggered, true);
  }

  willRemove() {
    this.element.removeEventListener(
      this.domEvent,
      this.onEventTriggered,
      true
    );
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants