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

[CLEANUP beta] Remove Application Controller Router Properties #19707

Merged
merged 1 commit into from
Aug 22, 2021
Merged

[CLEANUP beta] Remove Application Controller Router Properties #19707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 5 additions & 10 deletions ...ages/@ember/-internals/glimmer/tests/integration/components/link-to/routing-angle-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1726,16 +1726,11 @@ moduleFor(

await this.click('#about-link');

expectDeprecation(() => {
let currentRouteName = this.applicationInstance
.lookup('controller:application')
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this test was specifically testing deprecated behavior, I think the deprecated API was only used during test of a legitimate behavior we want to retain.

I believe the right approach here is to refactor the test to use the router service instead of currentRouteName via the application controller, remove the expectDeprecation( code, and retain the test.

Can you dig into it?

.get('currentRouteName');
assert.notEqual(
currentRouteName,
'about',
'link-to should not transition if target is not equal to _self or empty'
);
}, 'Accessing `currentRouteName` on `controller:application` is deprecated, use the `currentRouteName` property on `service:router` instead.');
assert.notEqual(
this.appRouter.currentRouteName,
'about',
'link-to should not transition if target is not equal to _self or empty'
);
}

async [`@test it accepts string/numeric arguments`](assert) {
Expand Down
15 changes: 5 additions & 10 deletions ...ages/@ember/-internals/glimmer/tests/integration/components/link-to/routing-curly-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1848,16 +1848,11 @@ moduleFor(

await this.click('#about-link > a');

expectDeprecation(() => {
let currentRouteName = this.applicationInstance
.lookup('controller:application')
Copy link
Member

Choose a reason for hiding this comment

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

Again, I this test should be retained with the other expectDeprecation left in place, but the code for the specific deprecated API removed here refactored.

.get('currentRouteName');
assert.notEqual(
currentRouteName,
'about',
'link-to should not transition if target is not equal to _self or empty'
);
}, 'Accessing `currentRouteName` on `controller:application` is deprecated, use the `currentRouteName` property on `service:router` instead.');
assert.notEqual(
this.appRouter.currentRouteName,
'about',
'link-to should not transition if target is not equal to _self or empty'
);
}

async [`@test it accepts string/numeric arguments`](assert) {
Expand Down
51 changes: 2 additions & 49 deletions packages/@ember/-internals/routing/lib/system/router.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import { privatize as P } from '@ember/-internals/container';
import { OutletState as GlimmerOutletState, OutletView } from '@ember/-internals/glimmer';
import { computed, get, notifyPropertyChange, set } from '@ember/-internals/metal';
import { computed, get, set } from '@ember/-internals/metal';
import { FactoryClass, getOwner, Owner } from '@ember/-internals/owner';
import { BucketCache } from '@ember/-internals/routing';
import RouterService from '@ember/-internals/routing/lib/services/router';
import { A as emberA, Evented, Object as EmberObject, typeOf } from '@ember/-internals/runtime';
import Controller from '@ember/controller';
import { assert, deprecate, info } from '@ember/debug';
import { APP_CTRL_ROUTER_PROPS, ROUTER_EVENTS } from '@ember/deprecated-features';
import { ROUTER_EVENTS } from '@ember/deprecated-features';
import EmberError from '@ember/error';
import { cancel, once, run, scheduleOnce } from '@ember/runloop';
import { DEBUG } from '@glimmer/env';
Expand Down Expand Up @@ -1735,53 +1735,6 @@ function updatePaths(router: EmberRouter) {
// actually been entered at that point.
return;
}
if (APP_CTRL_ROUTER_PROPS) {
if (!('currentPath' in appController)) {
Object.defineProperty(appController, 'currentPath', {
get() {
deprecate(
'Accessing `currentPath` on `controller:application` is deprecated, use the `currentPath` property on `service:router` instead.',
false,
{
id: 'application-controller.router-properties',
until: '4.0.0',
url:
'https://deprecations.emberjs.com/v3.x#toc_application-controller-router-properties',
for: 'ember-source',
since: {
enabled: '3.10.0-beta.1',
},
}
);
return get(router, 'currentPath');
},
});
}
notifyPropertyChange(appController, 'currentPath');

if (!('currentRouteName' in appController)) {
Object.defineProperty(appController, 'currentRouteName', {
get() {
deprecate(
'Accessing `currentRouteName` on `controller:application` is deprecated, use the `currentRouteName` property on `service:router` instead.',
false,
{
id: 'application-controller.router-properties',
until: '4.0.0',
url:
'https://deprecations.emberjs.com/v3.x#toc_application-controller-router-properties',
for: 'ember-source',
since: {
enabled: '3.10.0-beta.1',
},
}
);
return get(router, 'currentRouteName');
},
});
}
notifyPropertyChange(appController, 'currentRouteName');
}
}

function didBeginTransition(transition: Transition, router: EmberRouter) {
Expand Down
9 changes: 2 additions & 7 deletions packages/@ember/application/tests/reset_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { run } from '@ember/runloop';
import { get } from '@ember/-internals/metal';
import Controller from '@ember/controller';
import { Router } from '@ember/-internals/routing';
import { moduleFor, AutobootApplicationTestCase } from 'internal-test-helpers';
Expand Down Expand Up @@ -129,9 +128,7 @@ moduleFor(
let location = initialRouter.get('location');

assert.equal(location.getURL(), '/one');
expectDeprecation(() => {
assert.equal(get(initialApplicationController, 'currentPath'), 'one');
Copy link
Member

Choose a reason for hiding this comment

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

Similar to above, can this assertion be changed to assert the router service with currentRoutePath? https://api.emberjs.com/ember/3.27/classes/RouterService/properties/currentRouteName?anchor=currentRouteName

This is an integration test, and I wouldn't want to lose the coverage that Ember's routing system in general is up to date after a visit call.

}, 'Accessing `currentPath` on `controller:application` is deprecated, use the `currentPath` property on `service:router` instead.');
assert.equal(initialRouter.currentPath, 'one');

this.application.reset();

Expand Down Expand Up @@ -160,9 +157,7 @@ moduleFor(
);

assert.equal(location.getURL(), '/one');
expectDeprecation(() => {
assert.equal(get(applicationController, 'currentPath'), 'one');
Copy link
Member

Choose a reason for hiding this comment

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

Same here as elsewhere in this file.

}, 'Accessing `currentPath` on `controller:application` is deprecated, use the `currentPath` property on `service:router` instead.');
assert.equal(router.currentPath, 'one');
});
}

Expand Down
1 change: 0 additions & 1 deletion packages/@ember/deprecated-features/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,4 @@
// not the version that the feature will be removed.

export const ROUTER_EVENTS = !!'4.0.0';
export const APP_CTRL_ROUTER_PROPS = !!'3.10.0-beta.1';
export const ASSIGN = !!'4.0.0-beta.1';
70 changes: 17 additions & 53 deletions packages/ember/tests/routing/decoupled_basic_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,14 +79,6 @@ moduleFor(
});
}

get currentPath() {
let currentPath;
expectDeprecation(() => {
currentPath = this.applicationInstance.lookup('controller:application').get('currentPath');
Copy link
Member

Choose a reason for hiding this comment

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

...and again here, use the router service.

(I'm surprised these were not all refactored when the deprecation was added.)

}, 'Accessing `currentPath` on `controller:application` is deprecated, use the `currentPath` property on `service:router` instead.');
return currentPath;
}

async ['@test warn on URLs not included in the route set'](assert) {
await this.visit('/');

Expand All @@ -95,7 +87,7 @@ moduleFor(

['@test The Homepage'](assert) {
return this.visit('/').then(() => {
assert.equal(this.currentPath, 'home', 'currently on the home route');
assert.equal(this.appRouter.currentPath, 'home', 'currently on the home route');

let text = this.$('.hours').text();
assert.equal(text, 'Hours', 'the home template was rendered');
Expand All @@ -109,15 +101,15 @@ moduleFor(

return this.visit('/camelot')
.then(() => {
assert.equal(this.currentPath, 'camelot');
assert.equal(this.appRouter.currentPath, 'camelot');

let text = this.$('#camelot').text();
assert.equal(text, 'Is a silly place', 'the camelot template was rendered');

return this.visit('/');
})
.then(() => {
assert.equal(this.currentPath, 'home');
assert.equal(this.appRouter.currentPath, 'home');

let text = this.$('.hours').text();
assert.equal(text, 'Hours', 'the home template was rendered');
Expand Down Expand Up @@ -708,12 +700,12 @@ moduleFor(
1,
'The home template was rendered'
);
assert.equal(this.currentPath, 'home');
assert.equal(this.appRouter.currentPath, 'home');
});
}

['@test Redirecting from the middle of a route aborts the remainder of the routes'](assert) {
assert.expect(5);
assert.expect(4);

this.router.map(function () {
this.route('home');
Expand Down Expand Up @@ -750,21 +742,15 @@ moduleFor(
return this.visit('/').then(() => {
let router = this.applicationInstance.lookup('router:main');
this.handleURLAborts(assert, '/foo/bar/baz');
let currentPath;
expectDeprecation(() => {
currentPath = this.applicationInstance
.lookup('controller:application')
.get('currentPath');
}, 'Accessing `currentPath` on `controller:application` is deprecated, use the `currentPath` property on `service:router` instead.');
assert.equal(currentPath, 'home');
assert.equal(router.currentPath, 'home');
assert.equal(router.get('location').getURL(), '/home');
});
}

['@test Redirecting to the current target in the middle of a route does not abort initial routing'](
assert
) {
assert.expect(7);
assert.expect(6);

this.router.map(function () {
this.route('home');
Expand Down Expand Up @@ -805,21 +791,15 @@ moduleFor(

return this.visit('/foo/bar/baz').then(() => {
assert.ok(true, '/foo/bar/baz has been handled');
let currentPath;
expectDeprecation(() => {
currentPath = this.applicationInstance
.lookup('controller:application')
.get('currentPath');
}, 'Accessing `currentPath` on `controller:application` is deprecated, use the `currentPath` property on `service:router` instead.');
assert.equal(currentPath, 'foo.bar.baz');
assert.equal(this.appRouter.currentPath, 'foo.bar.baz');
assert.equal(successCount, 1, 'transitionTo success handler was called once');
});
}

['@test Redirecting to the current target with a different context aborts the remainder of the routes'](
assert
) {
assert.expect(7);
assert.expect(6);

this.router.map(function () {
this.route('home');
Expand Down Expand Up @@ -860,13 +840,7 @@ moduleFor(

return this.visit('/').then(() => {
this.handleURLAborts(assert, '/foo/bar/1/baz');
let currentPath;
expectDeprecation(() => {
currentPath = this.applicationInstance
.lookup('controller:application')
.get('currentPath');
}, 'Accessing `currentPath` on `controller:application` is deprecated, use the `currentPath` property on `service:router` instead.');
assert.equal(currentPath, 'foo.bar.baz');
assert.equal(this.appRouter.currentPath, 'foo.bar.baz');
assert.equal(
this.applicationInstance.lookup('router:main').get('location').getURL(),
'/foo/bar/2/baz'
Expand Down Expand Up @@ -901,19 +875,11 @@ moduleFor(

return this.visit('/foo/bar/baz').then(() => {
assert.ok(true, '/foo/bar/baz has been handled');
let applicationController = this.applicationInstance.lookup('controller:application');
let router = this.applicationInstance.lookup('router:main');

let currentPath;
expectDeprecation(() => {
currentPath = applicationController.get('currentPath');
}, 'Accessing `currentPath` on `controller:application` is deprecated, use the `currentPath` property on `service:router` instead.');
assert.equal(currentPath, 'foo.bar.baz');
assert.equal(router.currentPath, 'foo.bar.baz');
run(() => router.send('goToQux'));
expectDeprecation(() => {
currentPath = applicationController.get('currentPath');
}, 'Accessing `currentPath` on `controller:application` is deprecated, use the `currentPath` property on `service:router` instead.');
assert.equal(currentPath, 'foo.qux');
assert.equal(router.currentPath, 'foo.qux');
assert.equal(router.get('location').getURL(), '/foo/qux');
});
}
Expand Down Expand Up @@ -1405,10 +1371,10 @@ moduleFor(
});
}

['@test currentRouteName is a property installed on ApplicationController that can be used in transitionTo'](
['@test currentRouteName is a property installed on Router that can be used in transitionTo'](
assert
) {
assert.expect(36);
assert.expect(24);

this.router.map(function () {
this.route('index', { path: '/' });
Expand All @@ -1424,17 +1390,15 @@ moduleFor(
});

return this.visit('/').then(() => {
let appController = this.applicationInstance.lookup('controller:application');
let router = this.applicationInstance.lookup('router:main');

function transitionAndCheck(path, expectedPath, expectedRouteName) {
if (path) {
run(router, 'transitionTo', path);
}
expectDeprecation(() => {
assert.equal(appController.get('currentPath'), expectedPath);
assert.equal(appController.get('currentRouteName'), expectedRouteName);
}, /Accessing `(currentPath|currentRouteName)` on `controller:application` is deprecated, use the `(currentPath|currentRouteName)` property on `service:router` instead\./);

assert.equal(router.currentPath, expectedPath);
assert.equal(router.currentRouteName, expectedRouteName);
}

transitionAndCheck(null, 'index', 'index');
Expand Down
8 changes: 0 additions & 8 deletions packages/ember/tests/routing/model_loading_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,6 @@ moduleFor(
});
}

get currentPath() {
let currentPath;
expectDeprecation(() => {
currentPath = this.applicationInstance.lookup('controller:application').get('currentPath');
}, 'Accessing `currentPath` on `controller:application` is deprecated, use the `currentPath` property on `service:router` instead.');
return currentPath;
}

async ['@test warn on URLs not included in the route set'](assert) {
await this.visit('/');

Expand Down
Loading