Removed initializer. Create options in server init

[cah-brian-gantzler]

Removed the initializer to fix the implicit deprecation warning. Property options was being injected into the service. I believe to make testing easier. Moved the creation of the options property to the service init method. Fixed all the tests to allow for testing the effects of this property. Removed the user of get since its the tests (addon supports 3.16+ anyway)

The property options on the metrics service was public, but did not need to be. I left it public, but I believe it should be localized only to the init method as I believe that is its only reasonable use.

Fixes #277

[Turbo87]

does this still work properly with embroider?

[cah-brian-gantzler]

I do not know. Not sure how to test.

The original import of ../config/environment did not seem to work correctly when running in tests if I tried to use that in the service

[cah-brian-gantzler]

Yes this does work in embroider, safe, the addon doesnt work under optimized for various reasons, not sure if this is one of them or not. I created another PR to add embroider try scenarios #280 although I dont understand why some of the checks were cancelled.

[Turbo87]

@ef4 any thoughts on this?

[ef4]

This part is fine.

The part of this addon that isn't going to work with optimizations is _lookupAdapter. IMO, the simplest way to make it work would be to change the API so that users configure their adapters from somewhere in their own Javascript, and they import the adapters they want instead of using string names for them. Something like:

// app.js
import { configureMetricsAdapter } from 'ember-metrics';
import GoogleAnalyticsAdapter from 'ember-metrics/metrics-adapters/google-analytics';

configureMetricsAdapter(GoogleAnalyticsAdapter, { optionsGoHere });

Alternatively, you could probably keep the adapter config as it is, but send it into @embroider/macros setOwnConfig and use it to emit code in the addon that imports only the adapters that are needed.

[cah-brian-gantzler]

The point of this PR was to fix the deprecation of no implicit injection. Not to make it embroider compatible. I think the question was if the one change I made was compatible and Ed confirmed it is.

For a PR to make it embroider compatible Ed is absolutely correct that the lookup adapter would fail unless the consuming app did something. I would suggest that the app create a a metrics adapter (as Ed stated above) in the following file metrics-adapters/google-analytics.js containing

import BaseAdapter from 'ember-metrics/metrics-adapters/google-analytics';

export default class GoogleAnalytics extends BaseAdapter {
}

The app only creates the ones it uses, it gives a clear place where they can override options if needed and requires no code change to this addon. Just an update to the docs that you dont have to create empty exports like this in ember classic or embroider safe, but you do if you want your app to be embroider optimized

[cah-brian-gantzler]

Missed the point that really the factoryFor line would be for ember classic compatibility, once you go embroider optimized you would not configure the adapters in the environment.js at all but instead but the above method. The ability of being able to specify if they are enabled in production or dev would have to be done a different way, but it would be a small change that you are bascially opting into by going embroider optimized.

From docs (How often is this really used?)

To only activate adapters in specific environments, you can add an array of environment names to the config, as the environments key. Valid environments are:

development
test,
production
all (default, will be activated in all environments)

Any changes to make this add embroider optimized should be in another PR. I can look into that later if you want

[cah-brian-gantzler]

I think all the current concerns were addressed. Could we get the workflow run please

[cah-brian-gantzler]

Could I get this re-run and considered for approval please

[jherdman]

Build kicked.

[cah-brian-gantzler]

The code I believe runs under embroider optimized, the problem is the tests themselves do not. I removed the embroiderOptimized from the ember-try, but forgot to remove it from the import so it failed linting. Sorry. (Also needed to remove from the github workflow tests.yml)

Could you please re-run workflow.

[jherdman]

IMHO this belongs in your global git ignore. https://stackoverflow.com/a/7335487

[cah-brian-gantzler]

Thanks, created. Dont want to update the .gitignore and cause the build to need to be kicked off again. Can fix this up if I have to update anything or just before the PR is accepted.

[jherdman]

Build kicked. Apologies for taking so long.

[jherdman]

@Turbo87 @ef4 any issues with this PR? I'm a little out of my depth on this one with respect to Embroider.

[cah-brian-gantzler]

This PR is not meant to make this addon embroider compatible, just to remove the Deprecated Implicit Injections. I do agree that that change should not make this addon less embroider compatible and Ed already weighed in on that if you read the previous comments #279 (comment)

[cah-brian-gantzler]

This PR fixes the deprecation of https://github.com/ember-cli/eslint-plugin-ember/blob/master/docs/rules/no-implicit-service-injection-argument.md What are the next steps to get this PR merged?

[jherdman]

@cah-briangantzler I'm going to merge this. Can you push up the tweak to the gitignore file?

[cah-brian-gantzler]

Sure, give me a couple

[jherdman]

Many thanks!