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 deprecate-router-events support code #19749

Merged
merged 3 commits into from Nov 5, 2021
Merged

[CLEANUP beta] Remove deprecate-router-events support code #19749

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a638631
[CLEANUP beta] deprecate-router-events
xg-wang Sep 9, 2021
9446348
add docs/expected back
xg-wang Sep 9, 2021
a076e85
removed used test code
xg-wang Sep 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
48 changes: 1 addition & 47 deletions packages/@ember/-internals/routing/lib/system/route.ts
View file
Open in desktop
Expand Up @@ -22,8 +22,7 @@ import {
} from '@ember/-internals/runtime';
import { isProxy, lookupDescriptor, symbol } from '@ember/-internals/utils';
import Controller from '@ember/controller';
import { assert, deprecate, info, isTesting } from '@ember/debug';
import { ROUTER_EVENTS } from '@ember/deprecated-features';
import { assert, info, isTesting } from '@ember/debug';
import { dependentKeyCompat } from '@ember/object/compat';
import { once } from '@ember/runloop';
import { classify } from '@ember/string';
Expand Down Expand Up @@ -2448,49 +2447,4 @@ Route.reopen({
},
});

export let ROUTER_EVENT_DEPRECATIONS: any;
if (ROUTER_EVENTS) {
ROUTER_EVENT_DEPRECATIONS = {
on(name: string) {
this._super(...arguments);
let hasDidTransition = name === 'didTransition';
let hasWillTransition = name === 'willTransition';

if (hasDidTransition) {
deprecate(
'You attempted to listen to the "didTransition" event which is deprecated. Please inject the router service and listen to the "routeDidChange" event.',
false,
{
id: 'deprecate-router-events',
until: '4.0.0',
url: 'https://deprecations.emberjs.com/v3.x#toc_deprecate-router-events',
for: 'ember-source',
since: {
enabled: '3.11.0',
},
}
);
}

if (hasWillTransition) {
deprecate(
'You attempted to listen to the "willTransition" event which is deprecated. Please inject the router service and listen to the "routeWillChange" event.',
false,
{
id: 'deprecate-router-events',
until: '4.0.0',
url: 'https://deprecations.emberjs.com/v3.x#toc_deprecate-router-events',
for: 'ember-source',
since: {
enabled: '3.11.0',
},
}
);
}
},
};

Route.reopen(ROUTER_EVENT_DEPRECATIONS);
}

export default Route;
73 changes: 17 additions & 56 deletions packages/@ember/-internals/routing/lib/system/router.ts
View file
Open in desktop
Expand Up @@ -6,8 +6,7 @@ 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 { ROUTER_EVENTS } from '@ember/deprecated-features';
import { assert, info } from '@ember/debug';
import EmberError from '@ember/error';
import { cancel, once, run, scheduleOnce } from '@ember/runloop';
import { DEBUG } from '@glimmer/env';
Expand All @@ -20,7 +19,6 @@ import Route, {
hasDefaultSerialize,
RenderOptions,
ROUTE_CONNECTIONS,
ROUTER_EVENT_DEPRECATIONS,
} from './route';
import RouterState from './router_state';

Expand All @@ -47,10 +45,6 @@ function defaultDidTransition(this: EmberRouter, infos: PrivateRouteInfo[]) {
this.notifyPropertyChange('url');
this.set('currentState', this.targetState);

// Put this in the runloop so url will be accurate. Seems
// less surprising than didTransition being out of sync.
once(this, this.trigger, 'didTransition');

if (DEBUG) {
// @ts-expect-error namespace isn't public
if (this.namespace.LOG_TRANSITIONS) {
Expand All @@ -63,11 +57,8 @@ function defaultDidTransition(this: EmberRouter, infos: PrivateRouteInfo[]) {
function defaultWillTransition(
this: EmberRouter,
oldInfos: PrivateRouteInfo[],
newInfos: PrivateRouteInfo[],
transition: Transition
newInfos: PrivateRouteInfo[]
) {
once(this, this.trigger, 'willTransition', transition);

if (DEBUG) {
// @ts-expect-error namespace isn't public
if (this.namespace.LOG_TRANSITIONS) {
Expand Down Expand Up @@ -380,50 +371,22 @@ class EmberRouter extends EmberObject.extend(Evented) implements Evented {
});
}

// TODO: merge into routeDidChange
didTransition(infos: PrivateRouteInfo[]) {
if (ROUTER_EVENTS) {
if (router.didTransition !== defaultDidTransition) {
deprecate(
'You attempted to override the "didTransition" method which is deprecated. Please inject the router service and listen to the "routeDidChange" event.',
false,
{
id: 'deprecate-router-events',
until: '4.0.0',
url: 'https://deprecations.emberjs.com/v3.x#toc_deprecate-router-events',
for: 'ember-source',
since: {
enabled: '3.11.0',
},
}
);
}
}
assert(
'You attempted to override the "didTransition" method which has been deprecated. Please inject the router service and listen to the "routeDidChange" event.',
router.didTransition === defaultDidTransition
);
router.didTransition(infos);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we can take this a step further and make defaultDidTransition && defaultWillTransition the defacto method that is called when didTransition/willTransition is called on the PrivateRouter. I'm wondering if we can take everything in, say defaultDidTransition and just throw it in here and then get rid of the assert.

Copy link
Contributor

Choose a reason for hiding this comment

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

@xg-wang what do you think?

Copy link
Contributor Author

@xg-wang xg-wang Oct 23, 2021 •
edited

Choose a reason for hiding this comment

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

@snewcomer @nlfurniss sorry for the late reply.

The problem of putting everything from defaultDidTransition to here is that there won't be assertion thrown when people who used to override the didTransition. Before this PR there are warnings.

My thought is that we should internally migrate from router_js's didTransition to routeDidChange then major bump router_js to remove didTransition/willTransition. Meanwhile keep the defaultDidTransition.

}

willTransition(
oldInfos: PrivateRouteInfo[],
newInfos: PrivateRouteInfo[],
transition: Transition
) {
if (ROUTER_EVENTS) {
if (router.willTransition !== defaultWillTransition) {
deprecate(
'You attempted to override the "willTransition" method which is deprecated. Please inject the router service and listen to the "routeWillChange" event.',
false,
{
id: 'deprecate-router-events',
until: '4.0.0',
url: 'https://deprecations.emberjs.com/v3.x#toc_deprecate-router-events',
for: 'ember-source',
since: {
enabled: '3.11.0',
},
}
);
}
}
router.willTransition(oldInfos, newInfos, transition);
// TODO: merge into routeWillChange
willTransition(oldInfos: PrivateRouteInfo[], newInfos: PrivateRouteInfo[]) {
assert(
'You attempted to override the "willTransition" method which has been deprecated. Please inject the router service and listen to the "routeWillChange" event.',
router.willTransition === defaultWillTransition
);
router.willTransition(oldInfos, newInfos);
}

triggerEvent(
Expand Down Expand Up @@ -1406,7 +1369,7 @@ class EmberRouter extends EmberObject.extend(Evented) implements Evented {
```

@method didTransition
@public
@private
@since 1.2.0
*/
// Set with reopen to allow overriding via extend
Expand All @@ -1419,7 +1382,7 @@ class EmberRouter extends EmberObject.extend(Evented) implements Evented {
Triggers the router level `willTransition` hook.

@method willTransition
@public
@private
@since 1.11.0
*/
// Set with reopen to allow overriding via extend
Expand Down Expand Up @@ -1649,6 +1612,7 @@ export function triggerEvent(
if (ignoreFailure) {
return;
}
// TODO: update?
throw new EmberError(
`Can't trigger action '${name}' because your app hasn't finished transitioning into its first route. To trigger an action on destination routes during a transition, you can call \`.send()\` on the \`Transition\` object passed to the \`model/beforeModel/afterModel\` hooks.`
);
Expand Down Expand Up @@ -1868,7 +1832,4 @@ EmberRouter.reopen({
}),
});

if (ROUTER_EVENTS) {
EmberRouter.reopen(ROUTER_EVENT_DEPRECATIONS);
}
export default EmberRouter;
1 change: 0 additions & 1 deletion packages/@ember/deprecated-features/index.ts
View file
Open in desktop
Expand Up @@ -3,5 +3,4 @@
// These versions should be the version that the deprecation was _introduced_,
// not the version that the feature will be removed.

export const ROUTER_EVENTS = !!'4.0.0';
export const ASSIGN = !!'4.0.0-beta.1';
95 changes: 2 additions & 93 deletions packages/ember/tests/routing/decoupled_basic_test.js
View file
Open in desktop
Expand Up @@ -52,20 +52,10 @@ moduleFor(
console.error = originalConsoleError;
}

handleURLAborts(assert, path, deprecated) {
handleURLAborts(assert, path) {
run(() => {
let router = this.applicationInstance.lookup('router:main');
let result;

if (deprecated !== undefined) {
expectDeprecation(() => {
result = router.handleURL(path);
});
} else {
result = router.handleURL(path);
}

result.then(
router.handleURL(path).then(
function () {
assert.ok(false, 'url: `' + path + '` was NOT to be handled');
},
Expand Down Expand Up @@ -1055,59 +1045,6 @@ moduleFor(
);
}

['@test Router `willTransition` hook passes in cancellable transition'](assert) {
assert.expect(8);
this.router.reopen({
willTransition(_, _2, transition) {
assert.ok(true, 'willTransition was called');
if (transition.intent.url !== '/') {
transition.abort();
}
},
});

this.router.map(function () {
this.route('nork');
this.route('about');
});

this.add(
'route:loading',
Route.extend({
activate() {
assert.ok(false, 'LoadingRoute was not entered');
},
})
);

this.add(
'route:nork',
Route.extend({
activate() {
assert.ok(false, 'NorkRoute was not entered');
},
})
);

this.add(
'route:about',
Route.extend({
activate() {
assert.ok(false, 'AboutRoute was not entered');
},
})
);

let deprecation = /You attempted to override the "willTransition" method which is deprecated\./;

return expectDeprecationAsync(() => {
return this.visit('/').then(() => {
this.handleURLAborts(assert, '/nork', deprecation);
this.handleURLAborts(assert, '/about', deprecation);
});
}, deprecation);
}

['@test Aborting/redirecting the transition in `willTransition` prevents LoadingRoute from being entered'](
assert
) {
Expand Down Expand Up @@ -1197,34 +1134,6 @@ moduleFor(
});
}

async ['@test `didTransition` event fires on the router'](assert) {
assert.expect(3);

this.router.map(function () {
this.route('nork');
});

await this.visit('/');

let router = this.applicationInstance.lookup('router:main');
router.one('didTransition', function () {
assert.ok(true, 'didTransition fired on initial routing');
});

await this.visit('/');

router.one('didTransition', function () {
assert.ok(true, 'didTransition fired on the router');
assert.equal(
router.get('url'),
'/nork',
'The url property is updated by the time didTransition fires'
);
});

await this.visit('/nork');
}

['@test `activate` event fires on the route'](assert) {
assert.expect(4);

Expand Down
27 changes: 0 additions & 27 deletions packages/ember/tests/routing/model_loading_test.js
View file
Open in desktop
Expand Up @@ -29,33 +29,6 @@ moduleFor(
console.error = originalConsoleError;
}

handleURLAborts(assert, path, deprecated) {
run(() => {
let router = this.applicationInstance.lookup('router:main');
let result;

if (deprecated !== undefined) {
expectDeprecation(() => {
result = router.handleURL(path);
});
} else {
result = router.handleURL(path);
}

result.then(
function () {
assert.ok(false, 'url: `' + path + '` was NOT to be handled');
},
function (reason) {
assert.ok(
reason && reason.message === 'TransitionAborted',
'url: `' + path + '` was to be aborted'
);
}
);
});
}

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

Expand Down