Closed davismj closed 6 years ago
@huochunpeng This should fix the issue that you were running into. Feedback welcome.
Just a minor nit on the trailing/leading slash thing, other than that I don't see any issues either. I like this change, it's cleaner this way.
👍 passed both my test app and production app for fixing #605.
@fkleuver what trailing / leading slash thing?
@davismj I pointed that out in my review comment.
This line: ${router.baseUrl || ''}${instruction.getBaseUrl() || ''}
If router.baseUrl
doesn't have a trailing slash and instruction.getBaseUrl()
doesn't yield a leading slash (or they both have a slash) then stuff will choke. Are we sure that can't happen?
router.baseUrl is used to generate urls for a route configured on any given router. The baseUrl is set by a router's current parent instruction. In light of this, it was originally designed to update the baseUrl when the parent instruction was fully processed and accepted, on the commitChanges step. However, this has always been a problem for child routers using redirect. They cannot use the generate method to generate a new URL because the baseUrl hasn't yet been updated.
This test demands a totally different approach. Whenever the router creates a new instruction the baseUrl should be updated. This is a well defined solution if we never enter a state where a baseUrl was updated to a value that was never finalized. This is never the case since a child router is--as far as I know--tied to the parent route. If the parent route fails to activate, then the child router is discarded. It will have an invalid value on it, but it will never be used for any purpose.
I don't anticipate any breaking changes. This should answer some issues of unexpected behavior with the router, including #378. If someone was relying on this behavior it may cause issues, but it was never a promised or documented behavior.