Router(js) refactor (wip)

[knownasilya]

Putting this here for visibility, this is still a WIP, please don't merge.

---

Notes from slack:

Nathan Hammond
Couple of thoughts:
- I generally dislike things like `isSetup` because eventually everything has that check on it. I understand why it's there, but it'd be nice not to need it in the future.

Nathan Hammond
The volatile CPs are because you're aliasing properties that are outside of Ember's world. This is actually a use case for observers. Or possibly refactoring router.js to emit something we can watch for.

[knownasilya]

Did something that is crashing the tests now.. hum.

[knownasilya]

Maybe the CPs aren't update since they aren't volatile. Tried get and set, those return errors too.

[knownasilya]

I'm thinking it has something to do with stubbing/mocking in the tests..

testem.js:1025 Error while processing route: index Cannot read property 'name' of undefined TypeError: Cannot read property 'name' of undefined
    at Class._queryParamsFor (http://localhost:7357/4927/dist/ember.debug.js:36032:63)
    at forEachQueryParam (http://localhost:7357/4927/dist/ember.debug.js:36579:26)
    at Class._deserializeQueryParams (http://localhost:7357/4927/dist/ember.debug.js:35944:7)
    at getFullQueryParams (http://localhost:7357/4927/dist/ember.debug.js:35286:12)
    at getQueryParamsFor (http://localhost:7357/4927/dist/ember.debug.js:35298:27)
    at Class.paramsFor (http://localhost:7357/4927/dist/ember.debug.js:34292:25)
    at Class.deserialize (http://localhost:7357/4927/dist/ember.debug.js:34955:30)
    at applyHook (http://localhost:7357/4927/dist/ember.debug.js:46709:30)
    at UnresolvedHandlerInfoByParam.runSharedModelHook (http://localhost:7357/4927/dist/ember.debug.js:47160:20)
    at UnresolvedHandlerInfoByParam.getModel (http://localhost:7357/4927/dist/ember.debug.js:47385:19)

[jelhan]

I've fixed the failing tests. I can't make a pull request against your fork. Don't know why, but you can cherry-pick the two commits from here: jelhan@0973652 jelhan@d91bc2a

[knownasilya]

@jelhan 🎆

[btecu]

Could do let { _isActiveIntent, state } = this._router; and use those instead.

[luxzeitlos]

How is this related to that? Is there still help needed?

[knownasilya]

Yes related. Feel free to submit against my fork. Next step would be to create/use public APIs from router.js

[luxzeitlos]

how do you feel about moving to ts?

[knownasilya]

Moving what to TS? This refactor or router.js?

[chadhietala]

Just giving you a heads up I started adventuring on this journey so that we can implement the rest of emberjs/rfcs#95.

Also related:

• Move to TypeScript router.js#247
• Update Router JS #16888

Also branch when we realized we needed to push more things down into router.js

• https://github.com/emberjs/ember.js/commits/route-info

[chadhietala]

It would be amazing if someone could use the type declarations from router.js and convert ember-router to TS.

[knownasilya]

@chadhietala can you explain that last bulleted link and it's context? https://github.com/emberjs/ember.js/commits/route-info

[knownasilya]

@chadhietala I've rebased against the changes in master, waiting on travis.

[knownasilya]

What is you recommendation around using recognizer? Should that be imported explicitly or should the used methods be wrapped in ember router.

For example: router._routerMicrolib.recognizer.handlersFor(routeName);

Also, handlerInfos seems to be a thing that comes from router.js and is used in many places. Is that a public API?

[chadhietala]

Sorry was on holiday. So RouteInfo is a subset of HandlerInfo its going to become public API as described in the RFC.

[knownasilya]

@chadhietala any thoughts on route-recognizer? Should that be encapsulated by router.js or should ember use it directly?

[locks]

hey @knownasilya, is this work still valid?

[knownasilya]

I don't think so. Other people tackled refactors at the same time that it would take a while to unwind.