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

[ENG-4008] Unsilence deprecation 'ember-metal.get-with-default' #1660

Merged
merged 9 commits into from
Sep 28, 2022
Merged

[ENG-4008] Unsilence deprecation 'ember-metal.get-with-default' #1660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
45984ba
Upgrade addons and fix problems
brianjgeiger Sep 20, 2022
a481946
Merge branch 'develop' into feature/ember-metal.get-with-default
brianjgeiger Sep 22, 2022
9ea26c7
Prevent probabilistic build failures
brianjgeiger Sep 22, 2022
3a9136a
Merge branch 'develop' into feature/ember-metal.get-with-default
brianjgeiger Sep 26, 2022
8ff29d1
Merge branch 'develop' into feature/ember-metal.get-with-default
brianjgeiger Sep 28, 2022
5bc7869
Merge branch 'develop' into feature/ember-metal.get-with-default
brianjgeiger Sep 28, 2022
216654c
Merge branch 'develop' into feature/ember-metal.get-with-default
brianjgeiger Sep 28, 2022
f9e0ab1
Don't async unnecessarily
brianjgeiger Sep 28, 2022
4819b8d
Make it more obvious how async is happening
brianjgeiger Sep 28, 2022
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
4 changes: 2 additions & 2 deletions app/authenticators/osf-cookie.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ export default class OsfCookie extends Base {
return { id };
}

restore() {
async restore() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this an async method? There is no await here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly because the ember-simple-auth upgrade guide said to do so. https://github.com/mainmatter/ember-simple-auth/blob/master/guides/upgrade-to-v3.md#make-your-custom-session-store-asynchronous

Copy link
Contributor

Choose a reason for hiding this comment

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

In which case, should we return await this.authenticate()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I can see why it didn't complain, because it is returning a promise, but this is the more correct way to go.

const {
lastVerifiedUserId,
session: {
Expand All @@ -107,7 +107,7 @@ export default class OsfCookie extends Base {

// Check for a valid auth cookie.
// If it fails, the session will be invalidated.
return this.authenticate();
return await this.authenticate();
}

async _checkApiVersion() {
Expand Down
4 changes: 2 additions & 2 deletions app/decorators/check-auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import Route from '@ember/routing/route';
import { inject as service, Registry as ServiceRegistry } from '@ember/service';
import config from 'ember-get-config';
import SessionService from 'ember-simple-auth/services/session';

import { NotLoggedIn } from 'ember-osf-web/errors';
import CurrentUser from 'ember-osf-web/services/current-user';
import OsfSession from 'ember-osf-web/services/session';
import transitionTargetURL from 'ember-osf-web/utils/transition-target-url';

const {
Expand All @@ -30,7 +30,7 @@ export default function checkAuth<T extends ConcreteSubclass<Route>>(
) {
class AuthenticatedRoute extends RouteSubclass {
@service router!: ServiceRegistry['router'];
@service session!: SessionService;
@service session!: OsfSession;
@service currentUser!: CurrentUser;

async beforeModel(transition: any) {
Expand Down
8 changes: 8 additions & 0 deletions app/services/-private/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@

import SessionService from 'ember-simple-auth/services/session';

export default class OsfSession extends SessionService {
handleAuthentication() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect we can get away with rewriting this line to use session.handleAuthentication(transition.targetName) or something instead, so we can remove the ember-simple-auth.events.session-service deprecation handler. That may allow us to not have to stub the session service in our tests either?

return;
}
}
1 change: 1 addition & 0 deletions app/services/session.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default } from 'ember-osf-web/services/-private/session';
5 changes: 4 additions & 1 deletion config/deprecation-workflow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ self.deprecationWorkflow = self.deprecationWorkflow || {};
self.deprecationWorkflow.config = {
workflow: [
{ handler: 'silence', matchId: 'ember-inflector.globals' },
{ handler: 'silence', matchId: 'ember-metal.get-with-default' },
{ handler: 'silence', matchId: 'computed-property.volatile' },
{ handler: 'silence', matchId: 'implicit-injections' },
{ handler: 'silence', matchId: 'manager-capabilities.modifiers-3-13' },
Expand All @@ -18,5 +17,9 @@ self.deprecationWorkflow.config = {
{ handler: 'silence', matchId: 'ember-engines.deprecation-camelized-engine-names' },
{ handler: 'silence', matchId: 'ember-data:legacy-test-helper-support' },
{ handler: 'silence', matchId: 'has-block-and-has-block-params' },
{ handler: 'silence', matchId: 'ember-simple-auth.initializer.setup-session-restoration' },
{ handler: 'silence', matchId: 'ember-simple-auth.events.session-service' },
{ handler: 'silence', matchId: 'ember-cli-mirage.miragejs.import' },
{ handler: 'silence', matchId: 'ember-cli-mirage-config-routes-only-export' },
Comment on lines +20 to +23
Copy link
Contributor

Choose a reason for hiding this comment

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

So we un-silence one deprecation warning and add 4 more rules to silence deprecation warnings? Is it because removing the ember-metal.get-with-default warning requires upgrading ember-simple-auth?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, and mirage had to be upgraded as well.

],
};
18 changes: 10 additions & 8 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,8 @@
"ember-cli-htmlbars": "^5.7.1",
"ember-cli-inject-live-reload": "^2.0.2",
"ember-cli-inline-content": "0.4.1",
"ember-cli-meta-tags": "^6.1.2",
"ember-cli-mirage": "^2.0.1",
"ember-cli-meta-tags": "^7.0.0",
"ember-cli-mirage": "^2.4.0",
"ember-cli-moment-shim": "^3.5.0",
"ember-cli-password-strength": "^2.0.0",
"ember-cli-release": "^1.0.0-beta.2",
Expand All @@ -155,15 +155,15 @@
"ember-concurrency-ts": "^0.2.2",
"ember-content-placeholders": "https://github.com/cos-forks/ember-content-placeholders#master#2018-07-19",
"ember-cookies": "^0.4.0",
"ember-cp-validations": "https://github.com/GavinJoyce/ember-cp-validations#gj/ember-3.13",
"ember-cp-validations": "4.0.0-beta.13",
"ember-css-modules": "^1.0.3",
"ember-css-modules-reporter": "^1.2.0",
"ember-css-modules-sass": "^1.0.1",
"ember-css-modules-stylelint": "^1.2.0",
"ember-data": "~3.26.0",
"ember-decorators": "^6.1.1",
"ember-diff-attrs": "^0.2.1",
"ember-element-helper": "^0.5.5",
"ember-element-helper": "^0.6.1",
"ember-engines": "^0.8.13",
"ember-event-helpers": "^0.1.0",
"ember-exam": "^6.0.1",
Expand Down Expand Up @@ -192,7 +192,7 @@
"ember-radio-button": "^2.0.0",
"ember-resolver": "^8.0.2",
"ember-responsive": "^3.0.0",
"ember-simple-auth": "^2.1.1",
"ember-simple-auth": "^4.2.2",
"ember-sinon": "^5.0.0",
"ember-sinon-qunit": "^5.0.0",
"ember-sortable": "2.2",
Expand Down Expand Up @@ -228,7 +228,7 @@
"loaders.css": "^0.1.2",
"mime-types": "^2.1.22",
"mocha": "^6.1.4",
"node-sass": "^4.13.1",
"node-sass": "^4.14.0",
"npm-run-all": "^4.1.5",
"prettier": "^2.2.1",
"qunit": "^2.14.1",
Expand All @@ -245,7 +245,9 @@
},
"resolutions": {
"ember-cli-clipboard": "0.9.0",
"@embroider/macros": "^1.0.0"
"@embroider/macros": "^1.0.0",
"ember-element-helper": "^0.6.1",
"ember-validators": "^4.1.1"
},
"engines": {
"node": "10.* || >= 12"
Expand All @@ -265,7 +267,7 @@
]
},
"volta": {
"node": "10.18.0",
"node": "14.15.0",
"yarn": "1.21.1"
},
"dependencies": {}
Expand Down
14 changes: 10 additions & 4 deletions tests/integration/components/osf-navbar/component-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,24 +16,30 @@ const routerStub = Service.extend({
},
});

const sessionStub = Service.extend({
isAuthenticated: false,
on: () => { /* stub */ },
});

module('Integration | Component | osf-navbar', hooks => {
setupRenderingTest(hooks);

hooks.beforeEach(function(this: TestContext) {
// Make sure currentURL is always a string
this.owner.unregister('service:router');
this.owner.register('service:router', routerStub);
this.owner.register('service:session', sessionStub);
});

test('it renders', async assert => {
await render(hbs`{{osf-navbar}}`);
await render(hbs`<OsfNavbar />`);
assert.dom('.service-name').includesText('OSF');
assert.dom('.current-service').hasText('HOME');
});

test('service-dropdown: logged in', async function(assert) {
this.owner.lookup('service:session').set('isAuthenticated', true);
await render(hbs`{{osf-navbar}}`);
await render(hbs`<OsfNavbar />`);

assert.dom('[data-test-service-dropdown]').exists();

Expand All @@ -44,7 +50,7 @@ module('Integration | Component | osf-navbar', hooks => {
test('auth-dropdown: logged in', async function(assert) {
this.owner.lookup('service:session').set('isAuthenticated', true);

await render(hbs`{{osf-navbar}}`);
await render(hbs`<OsfNavbar />`);

assert.dom('[data-test-auth-dropdown-toggle]').exists();
await click('[data-test-auth-dropdown-toggle]');
Expand All @@ -54,7 +60,7 @@ module('Integration | Component | osf-navbar', hooks => {
test('osf-navbar: logged out', async function(assert) {
this.owner.lookup('service:session').set('isAuthenticated', false);

await render(hbs`{{osf-navbar}}`);
await render(hbs`<OsfNavbar />`);
assert.dom('[data-test-service-dropdown]').exists();

assert.dom('[data-test-auth-dropdown-toggle]').doesNotExist();
Expand Down
12 changes: 11 additions & 1 deletion tests/integration/components/tos-consent-banner/component-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import Service from '@ember/service';
import { render } from '@ember/test-helpers';
import { hbs } from 'ember-cli-htmlbars';
import { setupMirage } from 'ember-cli-mirage/test-support';
Expand All @@ -13,6 +14,12 @@ interface ThisTestContext extends TestContext {
currentUser: CurrentUser;
}

const sessionStub = Service.extend({
isAuthenticated: false,
data: {},
on: () => { /* stub */ },
});

module('Integration | Component | tos-consent-banner', hooks => {
setupRenderingTest(hooks);
setupMirage(hooks);
Expand All @@ -21,14 +28,17 @@ module('Integration | Component | tos-consent-banner', hooks => {
hooks.beforeEach(function(this: ThisTestContext) {
this.store = this.owner.lookup('service:store');
this.currentUser = this.owner.lookup('service:current-user');
this.owner.unregister('service:session');
this.owner.register('service:session', sessionStub);
Comment on lines +31 to +32
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is a similar issue I ran into in my router-deprecation PR. Since the session service is looked-up in the currentUser service, registering our session stub fails quietly, so we need to unregister it first. I'm hoping they (Ember community) adds some sort of warning that registering over an extant service fails :/


});

hooks.afterEach(() => {
sandbox.restore();
});

test('hidden when no user is logged in', async function(assert) {
await render(hbs`{{tos-consent-banner}}`);
await render(hbs`<TosConsentBanner />`);
assert.dom(this.element).hasText('');
});

Expand Down
Loading