[CLEANUP beta] Remove Application Controller Router Properties#19707
[CLEANUP beta] Remove Application Controller Router Properties#19707mixonic merged 1 commit intoemberjs:masterfrom
Conversation
mixonic
left a comment
There was a problem hiding this comment.
@btecu I think in the testing suite in general (both in places I flagged and even in places I didn't flag) the right path here is to refactor the assertions to use the router service public API instead of the application controller API. Removing the assertions (or tests) entirely is too much of a change, and degrades out test coverage of how Ember reflect routing state.
Can you give it another look through that lens?
|
|
||
| expectDeprecation(() => { | ||
| let currentRouteName = this.applicationInstance | ||
| .lookup('controller:application') |
There was a problem hiding this comment.
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?
|
|
||
| expectDeprecation(() => { | ||
| let currentRouteName = this.applicationInstance | ||
| .lookup('controller:application') |
There was a problem hiding this comment.
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.
|
|
||
| assert.equal(location.getURL(), '/one'); | ||
| expectDeprecation(() => { | ||
| assert.equal(get(initialApplicationController, 'currentPath'), 'one'); |
There was a problem hiding this comment.
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.
|
|
||
| assert.equal(location.getURL(), '/one'); | ||
| expectDeprecation(() => { | ||
| assert.equal(get(applicationController, 'currentPath'), 'one'); |
There was a problem hiding this comment.
Same here as elsewhere in this file.
| get currentPath() { | ||
| let currentPath; | ||
| expectDeprecation(() => { | ||
| currentPath = this.applicationInstance.lookup('controller:application').get('currentPath'); |
There was a problem hiding this comment.
...and again here, use the router service.
(I'm surprised these were not all refactored when the deprecation was added.)
4b70848 to
80b9a35
Compare
|
@mixonic updated. |
No description provided.