Deprecate EmberObject Usage?

[jherdman]

Each adapter inherits for EmberObject. Is it possible for us to just write native classes?

[jfdnc]

@jherdman I've got a proof-of-concept branch with a working refactor (all tests green) if you want to check it out: https://github.com/jfdnc/ember-metrics/tree/refactor/remove-ember-object

One of the things that I don't feel entirely sure about, just because I haven't used this API before, is using @ember/destroyable (and in particular registerDestructor) to maintain willDestroy functionality for adapter teardown. There are no tests present for that call currently, so I could spin those up and/or test this branch in a local project to QA that. If that solution works as expected, it seems like we can factor out EmberObject without changing the public API at all.

Also included in the linked branch is an in-place refactor/cleanup of the metrics service to improve readability of that file. Definitely open to feedback and/or opinions, and that is outside the scope of EmberObject removal, but we have some good opportunities there to simplify and make things more readable.

Any feedback is welcome, would love to be of help here if this seems like a plausible path forward.

[jherdman]

Some high level thoughts as I'm chewing on this still:

• I like your service refactor and GA refactor. Would you be amenable to making these their own PRs?
• Something that's concerned me about the native class refactor is that implementations of adapters might use the various API bits of Ember Object. Do we offer a deprecation release and THEN an upgrade release, or just cut over and offer an upgrade guide? Personally I'm leaning towards the latter. What are your thoughts?

Everything looks solid. Here are my next steps:

1. Read up on @ember/destroyable. I don't know much about this.
2. Smoke test this PR on a production app.

[jfdnc]

...service refactor and GA refactor. Would you be amenable to making these their own PRs?

Happy to, yep!

...Do we offer a deprecation release and THEN an upgrade release, or just cut over and offer an upgrade guide? Personally I'm leaning towards the latter. What are your thoughts?

I kept init and willDestroy in my refactor attempt to leave those hooks alone and not change the API, but I can see those moving to just constructor overrides and then maybe moving the teardown to onTeardown or something that destroyable can call. That sort of change kinda feels like a major version bump with at least a short upgrade guide (and requisite changes to the README, etc), though.

My guess is that the most common bits of EmberObject that will be used in local extensions are this._super (that will need to move to super.<method>), and then maybe this.get and this.set. Regardless, the native class change here would break any of those implicit dependencies, so I think even that level would have to be a major version bump.

Given those things, it might be the most responsible thing to do a deprecation release first before removing the underlying EmberObject. That way, we could also move away from init and willDestroy and provide a path for apps to refactor those hooks as well.

[jfdnc]

I guess willDestroy could still hang around, since its common in other Ember-land constructs, and is used in Glimmer components as well.

[jfdnc]

Another minor note in thinking about deprecations moving forward, the options property of the metrics service should also probably be phased out. Its only existing to reflect values that the consumer is already passing in from environment.js, and is not passed around internally after the service spins up.

[jherdman]

@jfdnc the Metrics service refactor has been merged. Are you interested in getting your GA refactor in too?

[jherdman]

@jfdnc I had another thought this morning I wanted to bounce off of you. A while back I investigated https://github.com/DavidWells/analytics as a possible underpinning for this library. One of the things I considered a blocker back then was that there is no way to remove an injected third party script for analytics from the DOM (DavidWells/analytics#78).

As far as I can tell the author's response is correct — just because we removed the script from the DOM doesn't mean that the code will stop running. Really we only do it here for testing.

This is today's shower thought: does it even make sense to continue supporting script tag removal?

[jfdnc]

continue supporting script tag removal?

It may make more sense to isolate the removal to just the testing environment. Given that those scripts are registering code that isn't torn down by removing the <script/> elements, I suppose it doesn't matter if they get torn down by default in the willDestroy hooks. The various adapters are also deleting the associated window keys for each script, and I can see cases where that might cause unexpected errors in the consumer, so it might also be a good idea to avoid that as a default behavior as well.

Either way, the willDestroy hook of the service should only fire on app teardown in a real application, so I don't think its a big practical concern. I think it makes sense to remove the defaults and just let the consumer override that hook if they need to (maybe an embedded ember-engines app? seems like a corner case though tbh). This would also include removing the willDestroy assertion and have that be optional API. We could also expose an attribute for targeting the registered script and the window object for the script to support this (see final comment below) .

I do think it is still a good idea to expose some method in the various adapters to hook into service teardown, since any given application might want to handle concerns at that time, especially if they are running a custom local adapter. One of the things I mentioned previously was maybe leveraging @ember/destroyable for this, but actually if we just expose any method name, the service can call that directly.

Current service initiates willDestroy hooks like:

willDestroy() {
    for (let adapter of this._adapters) {
      adapter.destroy();
    }
  }

The service's willDestroy will still be a fine place to kick off the adapter teardown since Service continues to have that hook, but if the adapters move to POJO classes, we could either keep the willDestroy name or expose a public method like onTeardown and call like

willDestroy() {
    for (let adapter of this._adapters) {
      adapter.willDestroy(); // or adapter.onTeardown()
    }
  }

and the behavior from a consuming app will remain the same.

Another related thought here is that the adapters are targeting those scripts with a CSS * selector that is maybe dangerous for the consumer. Calling querySelectorAll and removing script[src*=something] could have unintended side effects and remove scripts that were not attached by ember-metrics. I think it would be safer to generate a local key (maybe using guidFor) and attach a data-ember-metrics data attribute to each script, and then handle removal by targeting that specific node on teardown. If we deprecate the remove-scripts-and-window-objects-by-default behavior, each adapter could expose a generated selector id for the script, and a reference to the script's window object so the consumer can manually handle if needed.

[jherdman]

IMHO plain JavaScript is better, i.e. Base.detroy() with .willDestroy() exposed. Reduced cognitive overhead, etc.

Another related thought here is that the adapters are targeting those scripts with a CSS * selector that is maybe dangerous for the consumer.

Another problem we don't need to worry about if we just don't do it 😉

You're right though: if we're going to keep ripping the script tags out we need to do it safely. It's a bit challenging given the heterogeneous nature of script insertion — both on our end, and of the vendor themselves.

[jfdnc]

One way to attach markers for script removal would be like

import { guidFor } from '@ember/object/internals';
...
class MyCoolAdapter extends BaseAdapter {
  constructor() {
    // could technically just use component name like `data-ember-metrics-my-cool-adapter`
    // but generating it always ensures a unique target for removal per the adapter
    this.dataAttrForScript = `data-ember-metrics-${guidFor(this)}`
  }
  _injectScript() {
    // this looks different per the adapter
    // but generally we can find where `createElement` is called
    // and then add in `element.setAttribute(this.dataAttrForScript, '')`
    // so the element looks like `<script data-ember-metrics-0fxbaetc src="...`
  }
  willDestroy() {
    // could call here by default
    // or just expose `dataAttrForScript` (or whatever name) if the consumer wants to handle it
    removeFromDOM(`script[${this.dataAttrForScript}]`)
  }
}

[jherdman]

@jfdnc let's just decorate the script tags with data-ember-cli-metrics to keep things easy.

[GreatWizard]

I just saw the talk here. I probably shot to quick a PR about it: #331
My main target was to remove the ember-classic-decorator by using the native constructor.

It seems the solution implemented in the PR was already investigated by @jfdnc.
Should I close my PR or can we continue on this first step to use the native constructor?

[jfdnc]

@GreatWizard PR looking solid! I don't mind deferring my branch for PR, looks like you took a similar approach (I like the _activeAdapter change as well, I wasn't entirely sure how to handle refactoring that).

[jherdman]

I'll stop dragging my buns on a deprecation PR so that this can move forward. My apologies to you both.

[jherdman]

OK, the initial deprecation PR is merged. The next step is to deprecate the init method altogether. My gut feeling is that #316 is somehow related to this, but I don't fully grok Embroider yet. I'll open a new issue to discuss init. I think we can safely consider this resolved. I'll leave it open for another 48h for feedback.