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.

Sign up for GitHub

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

Removed initializer. Create options in server init #279

Merged
merged 4 commits into from
Jun 21, 2021
Merged

Removed initializer. Create options in server init #279

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6862e16
Added embroider safe ember try scenario
cah-brian-gantzler May 8, 2021
2cd1d29
Removed initializer. Create options in server init
cah-brian-gantzler May 6, 2021
4e20dd9
Removed the embroiderOptimized from the import and tests
cah-brian-gantzler May 29, 2021
52d6032
Remove the intellij specific files from the .gitignore
cah-brian-gantzler Jun 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ jobs:
- ember-canary
- ember-default-with-jquery
- ember-classic
- embroider-safe

steps:
- uses: actions/checkout@v2
Expand Down
4 changes: 4 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,7 @@ package-lock.json
/.node_modules.ember-try/
/bower.json.ember-try
/package.json.ember-try

# Intellij
.idea
ember-metrics.iml
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

15 changes: 14 additions & 1 deletion addon/services/metrics.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,14 @@ export default class Metrics extends Service {
*/
enabled = true;

/**
* Information about the active adapters from environment.js
*
* I think this could have been isolated to the init method only, but since
* was public before, would have been a breaking change
*/
options;

/**
* When the Service is created, activate adapters that were specified in the
* configuration. This config is injected into the Service as
Expand All @@ -49,8 +57,13 @@ export default class Metrics extends Service {
* @return {Void}
*/
init() {
const adapters = this.options.metricsAdapters || emberArray();
const owner = getOwner(this);
const config = owner.factoryFor('config:environment').class;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this still work properly with embroider?

Copy link
Contributor Author

@cah-brian-gantzler cah-brian-gantzler May 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ef4 any thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@cah-brian-gantzler cah-brian-gantzler May 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const {metricsAdapters = []} = config;
const {environment = 'development'} = config;
this.options = {metricsAdapters, environment}

const adapters = this.options.metricsAdapters || emberArray();
owner.registerOptionsForType('ember-metrics@metrics-adapter', { instantiate: false });
owner.registerOptionsForType('metrics-adapter', { instantiate: false });

Expand Down
16 changes: 0 additions & 16 deletions app/initializers/metrics.js
View file
Open in desktop

This file was deleted.

8 changes: 8 additions & 0 deletions config/ember-try.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"use strict";

const getChannelURL = require("ember-source-channel-url");
const { embroiderSafe } = require('@embroider/test-setup');

module.exports = async function () {
return {
Expand Down Expand Up @@ -74,6 +75,13 @@ module.exports = async function () {
},
},
},
embroiderSafe({
npm: {
devDependencies: {
"webpack": '^5.0.0',
},
},
}),
],
};
};
3 changes: 2 additions & 1 deletion ember-cli-build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,6 @@ module.exports = function(defaults) {
behave. You most likely want to be modifying `./index.js` or app's build file
*/

return app.toTree();
const { maybeEmbroider } = require('@embroider/test-setup');
return maybeEmbroider(app);
};
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
],
"devDependencies": {
"@ember/optional-features": "^2.0.0",
"@embroider/test-setup": "^0.40.0",
"babel-eslint": "^10.1.0",
"broccoli-asset-rev": "^3.0.0",
"ember-cli": "~3.21.2",
Expand Down
146 changes: 54 additions & 92 deletions tests/unit/services/metrics-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
import { set, get } from '@ember/object';
import { set } from '@ember/object';
import { module, test } from 'qunit';
import { setupTest } from 'ember-qunit';
import sinon from 'sinon';

const environment = 'test';
let sandbox, metricsAdapters, options;
let sandbox, service;

module('Unit | Service | metrics', function(hooks) {
setupTest(hooks);

hooks.beforeEach(function() {
sandbox = sinon.createSandbox();

metricsAdapters = [
service = this.owner.factoryFor('service:metrics').create();
service.activateAdapters([
{
name: 'GoogleAnalytics',
environments: ['all'],
Expand All @@ -34,12 +34,7 @@ module('Unit | Service | metrics', function(hooks) {
foo: 'bar'
}
}
];

options = {
metricsAdapters,
environment
};
]);
});

hooks.afterEach(function() {
Expand All @@ -48,31 +43,17 @@ module('Unit | Service | metrics', function(hooks) {
delete window.mixpanel;
});

test('it creates adapters with owners (for container/injection purposes)', function(assert) {
const service = this.owner.factoryFor('service:metrics').create({ options });

get(service, '_adapters.LocalDummyAdapter');
let owner = this.owner;

assert.ok(owner);
});

test('it activates local adapters', function(assert) {
const service = this.owner.factoryFor('service:metrics').create({ options });

assert.ok(get(service, '_adapters.LocalDummyAdapter'), 'it activated the LocalDummyAdapter');
assert.equal(get(service, '_adapters.LocalDummyAdapter.config.foo'), 'bar', 'it passes config options to the LocalDummyAdapter');
assert.ok(service._adapters.LocalDummyAdapter, 'it activated the LocalDummyAdapter');
assert.equal(service._adapters.LocalDummyAdapter.config.foo, 'bar', 'it passes config options to the LocalDummyAdapter');
});

test('#activateAdapters activates an array of adapters', function(assert) {
const service = this.owner.factoryFor('service:metrics').create({ options });

assert.ok(get(service, '_adapters.GoogleAnalytics'), 'it activated the GoogleAnalytics adapter');
assert.equal(get(service, '_adapters.GoogleAnalytics.config.id'), 'UA-XXXX-Y', 'it passes config options to the GoogleAnalytics adapter');
assert.ok(service._adapters.GoogleAnalytics, 'it activated the GoogleAnalytics adapter');
assert.equal(service._adapters.GoogleAnalytics.config.id, 'UA-XXXX-Y', 'it passes config options to the GoogleAnalytics adapter');
});

test('#activateAdapters is idempotent', function(assert) {
const service = this.owner.factoryFor('service:metrics').create({ options });
service.activateAdapters([
{
name: 'GoogleAnalytics',
Expand All @@ -96,17 +77,16 @@ module('Unit | Service | metrics', function(hooks) {
}
}
]);
assert.equal(get(service, '_adapters.GoogleAnalytics.config.id'), 'UA-XXXX-Y', 'it does not override the GoogleAnalytics adapter');
assert.equal(get(service, '_adapters.Mixpanel.config.token'), '0f76c037-4d76-4fce-8a0f-a9a8f89d1453', 'it does not override the Mixpanel adapter');
assert.equal(get(service, '_adapters.LocalDummyAdapter.config.foo'), 'bar', 'it does not override the LocalDummyAdapter');
assert.equal(service._adapters.GoogleAnalytics.config.id, 'UA-XXXX-Y', 'it does not override the GoogleAnalytics adapter');
assert.equal(service._adapters.Mixpanel.config.token, '0f76c037-4d76-4fce-8a0f-a9a8f89d1453', 'it does not override the Mixpanel adapter');
assert.equal(service._adapters.LocalDummyAdapter.config.foo, 'bar', 'it does not override the LocalDummyAdapter');
});

test('#invoke invokes the named method on activated adapters', function(assert) {
const service = this.owner.factoryFor('service:metrics').create({ options });
const MixpanelStub = sandbox.stub(window.mixpanel, 'identify');
const GoogleAnalyticsStub = sandbox.stub(window, 'ga');
const GoogleAnalyticsSpy = sandbox.spy(get(service, '_adapters.GoogleAnalytics'), 'identify');
const MixpanelSpy = sandbox.spy(get(service, '_adapters.Mixpanel'), 'identify');
const GoogleAnalyticsSpy = sandbox.spy(service._adapters.GoogleAnalytics, 'identify');
const MixpanelSpy = sandbox.spy(service._adapters.Mixpanel, 'identify');
const opts = {
userId: '1e810c197e',
name: 'Bill Limbergh',
Expand All @@ -123,10 +103,9 @@ module('Unit | Service | metrics', function(hooks) {
});

test('#invoke invokes the named method on a single activated adapter', function(assert) {
const service = this.owner.factoryFor('service:metrics').create({ options });
const GoogleAnalyticsStub = sandbox.stub(window, 'ga');
const GoogleAnalyticsSpy = sandbox.spy(get(service, '_adapters.GoogleAnalytics'), 'trackEvent');
const MixpanelSpy = sandbox.spy(get(service, '_adapters.Mixpanel'), 'trackEvent');
const GoogleAnalyticsSpy = sandbox.spy(service._adapters.GoogleAnalytics, 'trackEvent');
const MixpanelSpy = sandbox.spy(service._adapters.Mixpanel, 'trackEvent');
const opts = {
userId: '1e810c197e',
name: 'Bill Limbergh',
Expand All @@ -141,12 +120,11 @@ module('Unit | Service | metrics', function(hooks) {
});

test('#invoke invokes the named methods on a whitelist of activated adapters', function(assert) {
const service = this.owner.factoryFor('service:metrics').create({ options });
const MixpanelStub = sandbox.stub(window.mixpanel, 'identify');
const GoogleAnalyticsStub = sandbox.stub(window, 'ga');
const GoogleAnalyticsSpy = sandbox.spy(get(service, '_adapters.GoogleAnalytics'), 'identify');
const MixpanelSpy = sandbox.spy(get(service, '_adapters.Mixpanel'), 'identify');
const LocalDummyAdapterSpy = sandbox.spy(get(service, '_adapters.LocalDummyAdapter'), 'trackEvent');
const GoogleAnalyticsSpy = sandbox.spy(service._adapters.GoogleAnalytics, 'identify');
const MixpanelSpy = sandbox.spy(service._adapters.Mixpanel, 'identify');
const LocalDummyAdapterSpy = sandbox.spy(service._adapters.LocalDummyAdapter, 'trackEvent');
const opts = {
userId: '1e810c197e',
name: 'Bill Limbergh',
Expand All @@ -164,24 +142,21 @@ module('Unit | Service | metrics', function(hooks) {
});

test("#invoke doesn't error when asked to use a single deactivated adapter", function(assert) {
const service = this.owner.factoryFor('service:metrics').create({ options });
service.invoke('trackEvent', 'Trackmaster2000', {});
assert.ok(true, 'No exception was thrown');
});

test('#invoke invokes the named method on a single activated adapter with no arguments', function(assert) {
const service = this.owner.factoryFor('service:metrics').create({ options });
const GoogleAnalyticsStub = sandbox.stub(window, 'ga');
const GoogleAnalyticsSpy = sandbox.spy(get(service, '_adapters.GoogleAnalytics'), 'trackPage');
const GoogleAnalyticsSpy = sandbox.spy(service._adapters.GoogleAnalytics, 'trackPage');
service.invoke('trackPage', 'GoogleAnalytics');

assert.ok(GoogleAnalyticsSpy.calledOnce, 'it invokes the track method on the adapter');
assert.ok(GoogleAnalyticsStub.withArgs('send').calledOnce, 'it invoked the Google Analytics method');
});

test('#invoke includes `context` properties', function(assert) {
const service = this.owner.factoryFor('service:metrics').create({ options });
const GoogleAnalyticsSpy = sandbox.spy(get(service, '_adapters.GoogleAnalytics'), 'trackPage');
const GoogleAnalyticsSpy = sandbox.spy(service._adapters.GoogleAnalytics, 'trackPage');

set(service, 'context.userName', 'Jimbo');
service.invoke('trackPage', 'GoogleAnalytics', { page: 'page/1', title: 'page one' });
Expand All @@ -190,8 +165,7 @@ module('Unit | Service | metrics', function(hooks) {
});

test('#invoke does not leak options between calls', function(assert) {
const service = this.owner.factoryFor('service:metrics').create({ options });
const GoogleAnalyticsSpy = sandbox.spy(get(service, '_adapters.GoogleAnalytics'), 'trackPage');
const GoogleAnalyticsSpy = sandbox.spy(service._adapters.GoogleAnalytics, 'trackPage');

set(service, 'context.userName', 'Jimbo');
service.invoke('trackPage', 'GoogleAnalytics', { page: 'page/1', title: 'page one', callOne: true });
Expand All @@ -201,8 +175,7 @@ module('Unit | Service | metrics', function(hooks) {
});

test('it can be disabled', function(assert) {
const service = this.owner.factoryFor('service:metrics').create({ options });
const GoogleAnalyticsSpy = sandbox.spy(get(service, '_adapters.GoogleAnalytics'), 'trackPage');
const GoogleAnalyticsSpy = sandbox.spy(service._adapters.GoogleAnalytics, 'trackPage');

set(service, 'enabled', false);
service.invoke('trackPage', 'GoogleAnalytics', { page: 'page/1', title: 'page one' });
Expand All @@ -211,7 +184,6 @@ module('Unit | Service | metrics', function(hooks) {
});

test('it implements standard contracts', function(assert) {
const service = this.owner.factoryFor('service:metrics').create({ options });
delete window.mixpanel.toString;
sandbox.stub(window.mixpanel);
sandbox.stub(window, 'ga');
Expand All @@ -228,54 +200,44 @@ module('Unit | Service | metrics', function(hooks) {
});

test('it does not activate adapters that are not in the current app environment', function(assert) {
const service = this.owner.factoryFor('service:metrics').create({
options: {
metricsAdapters: [
{
name: 'GoogleAnalytics',
config: {
id: 'UA-XXXX-Y'
}
},
{
name: 'LocalDummyAdapter',
environments: ['production'],
config: {
foo: 'bar'
}
}
]
service.activateAdapters([
{
name: 'GoogleAnalytics',
config: {
id: 'UA-XXXX-Y'
}
},
environment
});
{
name: 'LocalDummyAdapter',
environments: ['production'],
config: {
foo: 'bar'
}
}
]);

assert.ok(get(service, '_adapters.GoogleAnalytics'), 'it activated the GoogleAnalytics adapter');
assert.notOk(get(service, '_adapters.LocalDummyAdapter'), 'it did not activate the LocalDummyAdapter');
assert.ok(service._adapters.GoogleAnalytics, 'it activated the GoogleAnalytics adapter');
assert.notOk(service._adapters.LocalDummyAdapter, 'it did not activate the LocalDummyAdapter');
});

test('when in FastBoot env, it does not activate adapters that are not FastBoot-enabled', function(assert) {
window.FastBoot = true;
const service = this.owner.factoryFor('service:metrics').create({
options: {
metricsAdapters: [
{
name: 'GoogleAnalytics',
config: {
id: 'UA-XXXX-Y'
}
},
{
name: 'LocalDummyAdapter',
config: {
foo: 'bar'
}
}
]
service.activateAdapters([
{
name: 'GoogleAnalytics',
config: {
id: 'UA-XXXX-Y'
}
},
environment
});
{
name: 'LocalDummyAdapter',
config: {
foo: 'bar'
}
}
]);

assert.notOk(get(service, '_adapters.GoogleAnalytics'), 'it did not activate the GoogleAnalytics adapter');
assert.ok(get(service, '_adapters.LocalDummyAdapter'), 'it activated the LocalDummyAdapter');
assert.notOk(service._adapters.GoogleAnalytics, 'it did not activate the GoogleAnalytics adapter');
assert.ok(service._adapters.LocalDummyAdapter, 'it activated the LocalDummyAdapter');
});
});
13 changes: 13 additions & 0 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2383,6 +2383,14 @@
ember-cli-htmlbars-inline-precompile "^2.1.0"
ember-test-waiters "^1.1.1"

"@embroider/test-setup@^0.40.0":
version "0.40.0"
resolved "https://registry.yarnpkg.com/@embroider/test-setup/-/test-setup-0.40.0.tgz#bff8e7d780c1ba1a349f9e51605c9668a8f2c65d"
integrity sha512-Ukk7KDviBd7Ro181WPGp+Sk/fZpD7Os0tJ5msf/2TsCUL631cAQKkUqGjQsva+jJYQnqbB3pxF1fR5fnY3x3ug==
dependencies:
lodash "^4.17.20"
resolve "^1.17.0"

"@eslint/eslintrc@^0.1.3":
version "0.1.3"
resolved "https://registry.yarnpkg.com/@eslint/eslintrc/-/eslintrc-0.1.3.tgz#7d1a2b2358552cc04834c0979bd4275362e37085"
Expand Down Expand Up @@ -7819,6 +7827,11 @@ lodash@^4.17.19:
resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.17.20.tgz#b44a9b6297bcb698f1c51a3545a2b3b368d59c52"
integrity sha512-PlhdFcillOINfeV7Ni6oF1TAEayyZBoZ8bcshTHqOYJYlrqzRK5hagpagky5o4HfCzzd1TRkXPMFq6cKk9rGmA==

lodash@^4.17.20:
version "4.17.21"
resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.17.21.tgz#679591c564c3bffaae8454cf0b3df370c3d6911c"
integrity sha512-v2kDEe57lecTulaDIuNTPy3Ry4gLGJ6Z1O3vE1krgXZNrsQ+LFTGHVxVjcXPs17LhbZVGedAJv8XZ1tvj5FvSg==

log-symbols@^2.2.0:
version "2.2.0"
resolved "https://registry.yarnpkg.com/log-symbols/-/log-symbols-2.2.0.tgz#5740e1c5d6f0dfda4ad9323b5332107ef6b4c40a"
Expand Down