implement router helpers#21447
Conversation
|
blast from the past! |
|
(ty) |
|
@kategengler how do we get emberjs/rfcs#391 on to the RFC advancement automation? |
|
@NullVoxPopuli I did it and it opened emberjs/rfcs#1199 For the future, the instructions are here https://github.com/emberjs/rfcs/blob/main/.github/READTHIS.md#trigger-opening-advancement-pryml |
|
I don't think any of these actually need to be internal helpers, with all the complexity of manually constructing references and consuming tags. They can be implemented as normal public-API-based helpers. |
Is there an example of this in the code base? @ef4 |
|
@knownasilya like, class helpers and plain function helpers -- should just work, even if the imports are a smidge different -- the new @ember/helper apis have a lot of plain function implementations |
| const castedModels = models as {}[]; | ||
| const isCurrentlyActive = | ||
| !isMissing(current) && | ||
| this.routing.isActiveForRoute(castedModels, queryParams, routeName, current); |
There was a problem hiding this comment.
do we need to use private APIs for these isActive checks? what about isActive on the router service?
There was a problem hiding this comment.
router.isActive only evaluates against the current router state, so it can't express "will this be active once the in-flight transition settles." That requires running the active-check against targetState specifically, which is what isActiveForRoute(models, qp, route, state) does. Those APIs aren't on the public RouterService today, so we mirror <LinkTo> and use the private -routing service.
There was a problem hiding this comment.
only evaluates against the current router state
i mean... if the route isn't active, isActive is false -- this is correct, yea?
here is the implementation -- which looks pretty thorough:
| // Also track currentRouteName: during loading/error substates the URL | ||
| // doesn't change but the active route name does, so isActive() alone | ||
| // (which only consumes currentURL) would miss those transitions. | ||
| void this.router.currentRouteName; |
There was a problem hiding this comment.
if this is truely needed shouldn't isActive already handle this?
There was a problem hiding this comment.
Ideally yes, but isActive only entangles with the currentURL tag (consumeTag(tagFor(this._router, 'currentURL')), see #19004). During loading/error substates the URL doesn't change but currentRouteName does, so without separately consuming currentRouteName the helper wouldn't recompute on those transitions. Entangling it manually here is the workaround; fixing it in RouterService.isActive itself would be a separate change.
There was a problem hiding this comment.
it sounds like you're describing isActive as having correct behavior tho?
I don't understand
| [routeName, ...models]: [string | null | undefined, ...unknown[]], | ||
| { queryParams }: { queryParams?: Record<string, unknown> } | ||
| ): boolean { | ||
| if (isMissing(routeName) || models.some(isMissing)) { |
There was a problem hiding this comment.
why early return here?
There was a problem hiding this comment.
RouterService.isActive casts routeName as string and passes it to routerMicrolib.isActiveIntent, which doesn't account for undefined/null (there's a comment to that effect in router-service.ts). This early return guards the loading state so we return false instead of throwing/misbehaving when the route name or a model isn't resolved yet — same guard <LinkTo>'s isLoading uses.
There was a problem hiding this comment.
we control all of these methods and can verify if the old comment from over 4 years ago is still accurate :p
| import { isMissing } from './-router-helpers-utils'; | ||
|
|
||
| export default function isLoading(routeName: unknown, ...models: unknown[]): boolean { | ||
| return !routeName || models.some(isMissing); |
There was a problem hiding this comment.
shouldn't this help need the Router service in some way?
There was a problem hiding this comment.
No — "loading" here is purely "is the route name or any model still null/undefined". This mirrors <LinkTo>'s isLoading getter, which likewise only inspects route/models and never touches the router. There's nothing to ask the router service.
There was a problem hiding this comment.
that doesn't make sense -- this is exported as public API, which means it needs to be reactive based on the routerservice -- given the jsdoc comments above, there is implied reactivity, hich this function presently has none of
| import { isMissing } from './-router-helpers-utils'; | ||
|
|
||
| export default class IsTransitioningInHelper extends Helper { | ||
| @service('-routing') declare private routing: RoutingService<Route>; |
There was a problem hiding this comment.
why the private routing thing?
There was a problem hiding this comment.
RouterService.isActive can only answer "is this route active in the current state" — it consumes currentURL and checks the current router state. isTransitioningIn needs to compare the current state against the in-flight targetState and ask whether the route would be active in that target. The public service exposes neither currentState/targetState nor isActiveForRoute(..., state), so we go through the private -routing service. This is exactly what <LinkTo> does for its ember-transitioning-in/-out classes (willBeActive/isActiveForState in link-to.ts).
5 participants
emberjs/rfcs#391