aurelia / router

A powerful client-side router.
MIT License
120 stars 115 forks source link

Fix: Router ignored the root when creating hrefs #601

Open rluba opened 6 years ago

rluba commented 6 years ago

Aurelia router generated absolute paths but did not append the root path (ie. <base href="…">). This is incorrect and caused all non-default click actions (eg. CMD+Click, Right-click-> "Open in new Tab/Browser", etc.) to navigate to the wrong URL (see issue #457) and also broke links for crawlers.

Normal clicks only worked because they are intercepted by Aurelia and forwarded to aurelia-browser-history, which contains a corresponding bug that treats all URLs as fragments instead of expecting the base path to be present in absolute paths.

This fix requires the corresponding fix for aurelia-browser-history or the history will forward incorrect fragments to the router.

CLAassistant commented 6 years ago

CLA assistant check
All committers have signed the CLA.

rluba commented 6 years ago

Don’t merge this PR yet. I’ve discovered that this solution doesn’t work in all cases and that there are more issues when root is set to something else than /. I’ll test it more thoroughly and keep you updated.

rluba commented 6 years ago

I discovered that redirects via route configurations, eg., {route: '', name: 'home', redirect: 'somepath'}, are broken when using my PR. But while trying to fix this, I discovered that these redirects are generally broken on master (as of 5fcb6f4a0a9c4e552f9894b517104a4b336ac760).

There are multiple issues:

  1. Since 487c33d50fa938c9e725180877f6510ecae89db5: TypeError: Cannot read property 'childRouter' of undefined is thrown because determineWhatToLoad(…) tries to interpret the Redirect plan as a regular plan .
  2. Since aafa070dece5de190b83c4cc56791d53f04a0acd: _buildNavigationPlan(…) calls _createNavigationInstruction(…) and then generate(…) on a router before it knows it’s baseURL, causing it to pass incorrect URLs to new Redirect(…). This probably cancels out when there’s no root set, but it prevents me from fixing the redirect when a root is set.

Are these issues already known?

rluba commented 6 years ago
  1. is fixed in a separate PR (#603)
  2. is fixed in 0737128

The test coverage for this module is pretty horrible. I’ll help bring it up in the future if we decide to stick with Aurelia.

davismj commented 6 years ago

The test coverage for this module is pretty horrible. I’ll help bring it up in the future if we decide to stick with Aurelia.

That is a generous assessment lol. Part of the reason Router dev has been slow recently is I find myself spending more time fixing the tests than getting to actual issues, and its very time consuming.

Thanks for your hard work. These are on my radar.

mreiche commented 5 years ago

Can someone please update this PR and merge it finally? This requirement is still unsatisfied.

bigopon commented 5 years ago

@mreiche We are waiting for this PR at #632 to get merged, and then I'll incorporate @rluba changes in a new PR, or would be even better if @rluba could create PR based on refactored code.

mreiche commented 4 years ago

632 has been merged a while ago.