fix(core): Remove trailing slash from routes

[mleanos]

Prevents transitioning to a URL with a trailing slash.

If a trailing slash is found in the transitioning URL, then the transition is aborted & the user is redirected to the path without the trailing slash.

Adds missing client-side route configuration tests.

Demonstrates that the various route configurations can handle a trailing slash in the URL, and resolve to the correct client route.

Fixes #1075

[mleanos]

@jloveland @vaucouleur How does this solution seem to you?

My fix assumes that we want to remove the trailing slash. However, maybe that's not the desired behavior? As I was exploring possible solutions, I didn't see a straight forward way of keeping the trailing slash, but resolving to the correct client route.

I don't see a good reason to keep the slash.

[mleanos]

I'm a lot happier with this solution. We can define a $urlRouterProvider Rule that removes the trailing slashes from the URL, and it applies to all routes. The Rule is only defined once, in the Core module route configuration.

In order for this to work, we have to load the Core module routes into the angular app, before we load any other routes. This required a change to the default configuration, to change the order of which we're loading the client-side module javascript files. Now the Core module files are loaded first; which really makes sense to me anyways. We probably should have been doing that to begin with.

@jloveland @vaucouleur @ilanbiala Review?

[ilanbiala]

The code seems pretty good. 👍

[mleanos]

@rhutchison pointed out that we can avoid manually ensuring that the Core module loads first; with my changes to the default asset configuration. If we register the Articles module with Core as a dependency; app.registerModule('articles', ['core']); Injecting Core as a dependency is probably the preferred approach. I'll update this PR accordingly.

[mleanos]

@codydaig @ilanbiala @rhutchison LGTY?

[ilanbiala]

LGTM.

[jloveland]

@mleanos should we add a comment above this line to indicate core is required to provide deterministic order..forcing core to load first?

[ilanbiala]

doesn't sound like a bad idea

[mleanos]

I'm not sure if this forces Core to load first though. I know my wording above indicates that it does. But now that I think about it, all this does is add Core as a dependency; it's sort of ambiguous as to whether Core is indeed loading first, or not.

Perhaps a comment more along the lines of Core being required for special route handling? This aspect of module dependency feels weird to me. But I guess it's the intention of Angular's module dependency injection. I really feel like we should be able to tell Angular to load Core first, or into each module. Rather than injecting it into every single module definition. However, I guess that's a bit off topic :)

[jloveland]

I like your suggestion..something like "Core module is required for special route handling"

I am not sure either about the module loading order, from the docs it describes how modules are asynchronously loaded:

Because modules do nothing at load time they can be loaded into the VM in any order and thus script loaders can take advantage of this property and parallelize the loading process.

but then, the docs describe how dependencies are loaded first:

Modules can list other modules as their dependencies. Depending on a module implies that the required module needs to be loaded before the requiring module is loaded. In other words the configuration blocks of the required modules execute before the configuration blocks of the requiring module.

[jloveland]

@mleanos one thing I noticed is that trying to navigate to a bad route on articles doesn't give a Page Not Found response as it does on the root.
for example, try http://0.0.0.0:3000/articles/foo it just shows blank page.
on http://0.0.0.0:3000/foo and http://0.0.0.0:3000/chat/foo it will show Page Not Found

[mleanos]

@jloveland I'll look into that in a bit. There's probably a console error in the browser. If so, what is it?

[mleanos]

Actually, just checked on the master branch, and I'm seeing the same behavior. No console errors, but the server is returning a 400 response, because of the invalid articleId param ("foo").

I'll see if there's an easy fix. It seems like a good idea to add a fix for that to this PR. WDYT?

[jloveland]

Seems like fixing the articles invalid article param would be small enough to be included here..

[mleanos]

@ilanbiala @jloveland I added the comment to just the Articles client module. I think this is where it's more applicable, and generally users will use Articles to base their other modules off of.

To fix the invalid ArticleId issue, I added a new service for implementing $http interceptors for 400 & 404 server errors. Let me know if this looks good to you; I partly feel this should be in it's own PR, but if you're all happy then I don't think it's a huge deal (just more to review).

[mleanos]

I reverted the commit with the $http Interceptors. I forgot why we hadn't implemented them for 404 & 400 already. We're sending back 400 & 404 error responses from the server, where we don't want to redirect the user. It's going to be a bit more involved to fix this issue. I'd rather tackle that in a separate issue, and PR.

@ilanbiala @jloveland SGTY?

[jloveland]

SGTM

[codydaig]

LGTM

[mleanos]

Merging since all the feedback has been addressed.