-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
ui kv routing fix #5650
ui kv routing fix #5650
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming this is temporary and will be replaced by the public router service once it works as advertised.
Even so, I'd say this router service is complex enough to require some test coverage.
@@ -61,8 +61,7 @@ export default Component.extend(FocusOnInsertMixin, { | |||
return `cert/${key}`; | |||
}, | |||
onEnter: function(val) { | |||
let baseKey = this.get('baseKey'); | |||
let mode = this.get('mode'); | |||
let { baseKey, mode } = this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So jealous of your native getters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I realized we could do destructuring like this now, it was a bit of a 🤯.
|
||
return true; | ||
}, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file makes me full of sads...but I get it, bugs are bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah same
@DingoEatingFuzz Yeah, the plan is for it to be temporary which is also why I just overrode the existing router service, but kept the same API. |
Hey! Didn't look too much at this myself, will leave it to @DingoEatingFuzz , but I read the description and I love the way you've decided how to shimmy this in and back out again once its fixed, schweeet! |
* fix passing initialKey to the top level secret create page * add service that uses the private routing service because of various bugs * make custom router service more like the bundled router service * clear the store cache when the model changes
Creating a new secret from the nav input on a secrets list page had a few issues:
1 - it didn't take into account the partial filter if there was one (this is because we moved all secret creation to the root route instead of doing nested creates).
2 - The input didn't focus on route change so using it became trickier - if you navigated by typing in a key with a slash, you had to manually re-focus the input
3 - Sometimes you'd get the default query params for all of the routes showing up in the URL when you navigated - this is apparently the desired behavior of the new router service in ember, but the way to opt out has not been implemented - (see emberjs/ember.js#16973)
4 - The router service also causes more than the current model hook to re-fire so more of the template was re-rendering, sometimes causing uncaught errors (see emberjs/ember.js#16349 and emberjs/ember.js#15801)
Because of 3 and 4, this PR introduces a new router service that calls methods on the private
-routing
service that does not have these bugs (this is what we were doing pre 2.15 when the public router service was introduced). This is done in a way that once these bugs are worked out in the public router service, deletingservice/router.js
will again use the public routing service and we won't need to change the usage of that service throughout the application. Some of it is copied verbatim from the routing service, some of it just proxies to the existing private service.