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.

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

[FEATURE Router Service] Deprecate touching transition state #17038

Merged
merged 1 commit into from
Oct 4, 2018

Conversation

chadhietala
Copy link
Contributor

This deprecates access to the state, params and queryParams fields on the Transition that were private but became defacto public API since there was no other way to access it. The router service will expose RouteInfo objects which is the public object that has this state on it.

@chadhietala chadhietala mentioned this pull request Oct 3, 2018
16 tasks
@@ -219,7 +226,7 @@ class Route extends EmberObject implements IRoute {
}

let transition = this._router._routerMicrolib.activeTransition;
let state = transition ? transition.state : this._router._routerMicrolib.state;
let state = transition ? transition[STATE_SYMBOL] : this._router._routerMicrolib.state;
Copy link
Member

Choose a reason for hiding this comment

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

Need svelting here I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you might be confused. transition.state is still needed internally but access to it from application code should be deprecated and we just tell people to use the RouteInfo. All of these symbols are just allowing the integration of ember-router and router.js to work. I could have simply renamed the method but I felt that using a faux-symbol may deter people further. In the future we should just make these real symbols.

Copy link
Member

Choose a reason for hiding this comment

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

👍 thank you!

some of the comments though are specifically about deprecations, and should still apply

@@ -899,10 +906,10 @@ class Route extends EmberObject implements IRoute {

if (transition) {
// Update the model dep values used to calculate cache keys.
stashParamNames(this._router, transition.state!.routeInfos);
stashParamNames(this._router, transition[STATE_SYMBOL]!.routeInfos);
Copy link
Member

Choose a reason for hiding this comment

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

svelting?


let cache = this._bucketCache;
let params = transition.params;
let params = transition[PARAMS_SYMBOL];
Copy link
Member

Choose a reason for hiding this comment

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

Svelting?

@@ -1152,7 +1159,7 @@ class Route extends EmberObject implements IRoute {
if (transition.resolveIndex < 1) {
return;
}
return transition.state!.routeInfos[transition.resolveIndex - 1].context;
return transition[STATE_SYMBOL]!.routeInfos[transition.resolveIndex - 1].context;
Copy link
Member

Choose a reason for hiding this comment

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

Svelting? What do we return after the deprecation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No where this is returning the context aka the result of the model hook

@@ -73,6 +76,56 @@ function defaultWillTransition(
}
}

if (TRANSITION_STATE) {
Copy link
Member

Choose a reason for hiding this comment

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

svelte

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a svelte import.

Copy link
Member

Choose a reason for hiding this comment

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

DOH!

@chadhietala chadhietala merged commit 71fec6e into master Oct 4, 2018
@chadhietala chadhietala deleted the transition-hand-slapping branch October 4, 2018 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants