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

Jump to bottom

Explicitly lookup session and store services #2306

Closed
wants to merge 2 commits into from
Closed

Explicitly lookup session and store services #2306

wants to merge 2 commits into from

Conversation

BobrImperator
Copy link
Collaborator

closes #2302

  • remove setup-session-service initializer
  • add computed property to internal session to lazily lookup session-store.
  • add computed property to session service to lazily lookup session:main.
  • modifies tests to inject owner to entities that need to lookup dependencies.

@BobrImperator BobrImperator force-pushed the 2302-implicit-injection-deprecated branch from b8fd72f to 62810c2 Compare June 9, 2021 00:12
BobrImperator
BobrImperator commented Jun 9, 2021
View reviewed changes
packages/ember-simple-auth/tests/unit/session-stores/shared/storage-event-handler-behavior.js Outdated
Comment on lines 55 to 56
store.persist({ someOtherKey: 'value' }); // this data will be read again when the event is handled so that no change will be detected
window.dispatchEvent(new StorageEvent('storage', { someOtherKey: store.get('key') }));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed the keys because it seemed like there are some race conditions with other async tests.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I'd suggest looking into this again – we shouldn't have race conditions in the test suite.

@BobrImperator BobrImperator force-pushed the 2302-implicit-injection-deprecated branch 2 times, most recently from 36d5cfd to 808abc9 Compare June 9, 2021 00:19
@BobrImperator BobrImperator force-pushed the 2302-implicit-injection-deprecated branch from 808abc9 to 917ff7a Compare June 9, 2021 00:19
@BobrImperator BobrImperator self-assigned this Jun 9, 2021
marcoow
marcoow reviewed Jun 9, 2021
View reviewed changes
packages/ember-simple-auth/tests/unit/services/session-test.js
sessionService = Session.create({ session });
setOwner(sessionService, this.owner);
this.owner.register('service:session', sessionService, { instantiate: false });
session = this.owner.lookup('session:main');
Copy link
Member

Choose a reason for hiding this comment

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

Is this used anywhere?

packages/ember-simple-auth/tests/unit/services/session-test.js
this.owner.register('service:session', sessionService, { instantiate: false });
session = this.owner.lookup('session:main');
this.owner.register('service:session', Session);
sessionService = this.owner.lookup('service:session');
Copy link
Member

Choose a reason for hiding this comment

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

Is this used anywhere?

packages/ember-simple-auth/tests/unit/mixins/application-route-mixin-test.js
this.owner.register('session-store:test', EphemeralStore);

this.owner.register('session:main', InternalSession);
session = this.owner.lookup('session:main');
Copy link
Member

Choose a reason for hiding this comment

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

Is this used anywhere?

packages/ember-simple-auth/tests/unit/internal-session-store-test.js Outdated
sinon = sinonjs.createSandbox();

this.owner.register('session-store:test', EphemeralStore);
session = InternalSession.create(this.owner.ownerInjection());
Copy link
Member

Choose a reason for hiding this comment

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

Does

Suggested change
session = InternalSession.create(this.owner.ownerInjection());
session = this.owner.lookup('session:main');

not work here?

packages/ember-simple-auth/tests/unit/internal-session-store-test.js Outdated
Ember.testing = true; // eslint-disable-line ember/no-ember-testing-in-module-scope

this.owner.register('session-store:test', EphemeralStore);
session = InternalSession.create(this.owner.ownerInjection());
Copy link
Member

Choose a reason for hiding this comment

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

Does

Suggested change
session = InternalSession.create(this.owner.ownerInjection());
session = this.owner.lookup('session:main');

not work here?

packages/ember-simple-auth/tests/unit/internal-session-store-test.js Outdated
it('looks up the test session store when Ember.testing true', function() {
Ember.testing = true; // eslint-disable-line ember/no-ember-testing-in-module-scope

this.owner.register('session-store:test', EphemeralStore);
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary at all?

packages/ember-simple-auth/tests/unit/internal-session-store-test.js Outdated
Ember.testing = false; // eslint-disable-line ember/no-ember-testing-in-module-scope
sinon = sinonjs.createSandbox();

this.owner.register('session-store:test', EphemeralStore);
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary at all?

packages/ember-simple-auth/tests/unit/internal-session-store-test.js Outdated

it('looks up the application session store when Ember.testing false', function() {
Ember.testing = false; // eslint-disable-line ember/no-ember-testing-in-module-scope
sinon = sinonjs.createSandbox();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sinon = sinonjs.createSandbox();

The sandbox is already set up in the beforeEach.

packages/ember-simple-auth/tests/unit/mixins/application-route-mixin-test.js
@@ -24,8 +24,11 @@ describe('ApplicationRouteMixin', () => {
beforeEach(function() {
sinon = sinonjs.createSandbox();

session = InternalSession.create({ store: EphemeralStore.create() });
this.owner.register('session:main', session, { instantiate: false });
this.owner.register('session-store:test', EphemeralStore);
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary?

packages/ember-simple-auth/tests/unit/mixins/application-route-mixin-test.js
this.owner.register('session:main', session, { instantiate: false });
this.owner.register('session-store:test', EphemeralStore);

this.owner.register('session:main', InternalSession);
Copy link
Member

Choose a reason for hiding this comment

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

This should not be necessary since it's done in the initializer?

@BobrImperator BobrImperator mentioned this pull request Jun 11, 2021
Closed
3 tasks
@BobrImperator
Copy link
Collaborator Author

closed in favor of #2315

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcoow marcoow marcoow left review comments

@pichfl pichfl pichfl left review comments

Assignees

@BobrImperator BobrImperator

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implicit Injection Deprecated
3 participants
@BobrImperator @marcoow @pichfl