Router Service's `currentURL` does not include `rootURL`

[chancancode]

I think this is basically re-opening #16851

The issue is that currentURL does not include the rootURL, where as the other APIs added by the router service RFC (such as urlFor) are explicitly spec-ed to include it.

Given that the motivating use case for the router service is to re-build <LinkTo>, it seems not very likely that it would be correct to use currentURL without rootURL (checking if a link is active maybe?). It is also a bit strange to assume that the RFC would disagree with itself on what "URL" means without calling it out.

To investigate:

• Did the RFC intend the current behavior? Are there any good use cases for it?
• Can we still fix it? Is it too late?
• What do we do? Deprecate and come up with a new name?

[rwjblue]

Did the RFC intend the current behavior? Are there any good use cases for it?

I think we need @ef4 to help us out here.

Can we still fix it? Is it too late?

It seems really hard to know if this is heavily used in app code, simple searches will often turn up the currentURL() test helper.

What do we do? Deprecate and come up with a new name?

Probably.

[ef4]

I don't think this is intentional. My preference would be that all methods talking about URLs include the rootURL, whereas if people want to know the abstract route they should ask for the route name.

HOWEVER: it's incorrect to implement <LinkTo> using currentURL anyway. It should be using the dynamically scoped RouteInfo instead. Because the timing of when the value changes matters, both for people who are trying to control when links turn active and for cross-route animations.

[rwjblue]

HOWEVER: it's incorrect to implement <LinkTo> using currentURL anyway. It should be using the dynamically scoped RouteInfo instead

Confirm, the route info (nee "outlet state") is how we intend for all of the template land helpers to work (those helpers were RFC'ed and merged in a separate RFC from the router service itself).

[mehulkar]

transitionTo doesn’t work with rootURL either. But recognize does, which is unfortunate, because for links we get from external sources, we have to first add the rootURL to recognize them and make sure they’re valid, and then remove the rootURL to be able to transition to them.

[buschtoens]

Just got bitten by the exact behavior described by @mehulkar.

I'd also prefer that all properties / methods dealing with URLs always use the absolute URL, including the rootURL. Is this something we can shoehorn in, without requiring a major version bump?

[rwjblue]

In general, I think that we can fix this compatibly (as a bug fix) if someone has the time to dig into it.

[xg-wang]

Let's say if we've corrected the semantic for currentURL to return the absolute url, what should happen when transition to any url that's not prefixed by rootURL?

routerService.transitionTo('/relative-to-root-url')

Should router prefix a rootURL and transition to ${rootURL}/relative-to-root-url or do an external navigation?

[mehulkar]

@xg-wang thinking through it... this is the list of possibilities I can think of:

example	type	behavior
'foo'	routeName	internal transition
'/foo'	relative URL	??
'http://example.com/foo'	fully qualified URL	window.location.href =
'${currentDomain}/foo'	full qualified, same domain	internal transition

If a relative path is passed, personally I would think it should throw an error? I can't think of cases where I would want to transition with a relative URL, but without the full domain. Currently though, a leading slash means that the router thinks it resembles a URL:

• https://github.com/emberjs/ember.js/blob/v3.17.3/packages/@ember/-internals/routing/lib/services/router.ts#L132
• https://github.com/emberjs/ember.js/blob/v3.17.3/packages/@ember/-internals/routing/lib/utils.ts#L187-L194

[xg-wang]

@mehulkar Reading "Absolute URLs vs relative URLs" from https://developer.mozilla.org/en-US/docs/Learn/Common_questions/What_is_a_URL it seems full qualified, same domain is also an absolute URL.

I think we can likely add a routerService.transitionToURL() which fully respects how URL works. Then deprecate urls in transitionTo (basically anything starts with /)

When you're at https://example.com/foo

example	type	behavior
bar	relative URL	internal transition to /foo/bar
../zoo	relative URL	internal transition to /zoo
https://example.com/foo	Full URL	window.location.href=
//example.com/foo	Implicit protocol	window.location.href=
http://example.com/foo	fully qualified URL	window.location.href =
/foo	Implicit domain name	*window.location.href=

[*] should Implicit domain name also be treated as internal transition? Seems somewhat confusing.

---

Maybe for implicit domain name, do an internal transition if prefix mathes rootURL, otherwise a full navigation to the absolute URL, with current domain.

[rwjblue]

@ef4 - What do you think? Is this something we are just stuck with?

[buschtoens]

I think the extra scenarios laid out by @mehulkar and @xg-wang are worthwhile considering for future iterations, but would unnecessarily increase the scope of what could essentially be a bug fix now.

For now, when referring to "URL", I think what we actually mean is an absolute path-reference as per RFC 3986 section 4.2, so just an absolute path that starts with a single slash character and includes the rootURL.

We could at the very least fix transitionTo(url) and replaceWith(url) to expect the url to be prefixed with the rootURL. If the url matches a route but is not prefixed with rootURL, it should implicitly add the prefix and throw a deprecation warning. This way we don't break any existing apps (yet) that have come to rely on this quirk.

The next step IMO should be prefixing currentURL with rootURL as well, but this will most likely break apps. Alternatively we add a new property and deprecate currentURL. For instance, we could stop using the url lingo in favor of path, since we are essentially dealing with paths here.

I'd be down to implement at least the transitionTo(url) / replaceWith(url) fix. This bug is really annoying me. 😅

[mehulkar]

@buschtoens I'd be fine with that, but it would break our code of prepending rootURL in some cases. Personally, I'm fine with that (and I think I have some test cases that would catch the breakage? maybe?), but I wonder if a optional feature is the way to go here?

[buschtoens]

it would break our code of prepending rootURL in some cases.

Can you give an example? Are you referring to the proposed changes to transitionTo(url) / replaceWith(url) or to the proposed follow-up changes to currentURL (to include the rootURL prefix)?

I agree that the latter one (currentURL) would be very much breaking. But the fixes to transitionTo(url) / replaceWith(url) should be backwards-compatible:

If the url matches a route but is not prefixed with rootURL, it should implicitly add the prefix and throw a deprecation warning. This way we don't break any existing apps (yet) that have come to rely on this quirk.

[mehulkar]

Oh hmm... yeah I read that wrong. I can't think of any cases where we want to route explicitly without the rootURL. Sorry for the noise!

[xg-wang]

@buschtoens The fix sounds correct to me. One of our app has a util to strip rootURL from the param passed to transitionTo, the deprecation you proposed should trigger a warning for that case.

[rwjblue]

We could at the very least fix transitionTo(url) and replaceWith(url) to expect the url to be prefixed with the rootURL. If the url matches a route but is not prefixed with rootURL, it should implicitly add the prefix and throw a deprecation warning. This way we don't break any existing apps (yet) that have come to rely on this quirk.

Agree, this sounds like a good path forward to me.

[buschtoens]

I've published ember-routing-utils in the meantime, which includes removeRootURL(url) and prefixRootURL(url) methods.