Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

Routes with trailing / don't resolve properly #1075

Closed
jloveland opened this issue Nov 28, 2015 · 7 comments · Fixed by #1238
Closed

Routes with trailing / don't resolve properly #1075

jloveland opened this issue Nov 28, 2015 · 7 comments · Fixed by #1238
Assignees

Comments

@jloveland
Copy link
Contributor

routes for articles with trailing / should show articles, but doesn't..
http://localhost:3000/articles/

see below:
screen shot 2015-11-28 at 6 37 41 pm

@mleanos
Copy link
Member

mleanos commented Nov 29, 2015

This method is being invoke because the UI router is resolving this route as the articles.view state, and expecting a articleId parameter to be present.
https://github.com/meanjs/mean/blob/master/modules/articles/client/controllers/articles.client.controller.js#L78

The Articles API is resolving correctly to the articles.list server controller method, because of how the Angular service is sending the request. However, it's sending back an array & the Articles service is expecting this call to be an object. Angular is complaining about this..
routes-with-trailing-slashes-gh-1075

I think the only viable option is to redirect the user back to the articles.list state. I don't see a need to reconcile the trailing / with the UI router to use the list templateUrl.

@vaucouleur
Copy link
Contributor

@mleanos @jloveland

There is a FAQ entry with respect to trailing slashes, would it be appropriate to use the method that ui-router recommends ?

https://github.com/angular-ui/ui-router/wiki/Frequently-Asked-Questions#how-to-make-a-trailing-slash-optional-for-all-routes

@vaucouleur
Copy link
Contributor

I gave a quick try today at the recommended approach in the ui-router faq, and it did not seem directly applicable. May be some interference with the html5 mode, or with the abstract state, not quite sure at this point. I ended up with either double slashes, or the route with the slash working but not the one without the slash, etc. I suggest this should be tagged as a bug @codydaig

@mleanos
Copy link
Member

mleanos commented Feb 27, 2016

@vaucouleur @jloveland What do you think of this approach?

  getArticle.$inject = ['$state', '$stateParams', '$timeout', 'ArticlesService'];

  function getArticle($state, $stateParams, $timeout, ArticlesService) {
    if ($stateParams.articleId) {
      return ArticlesService.get({
        articleId: $stateParams.articleId
      }).$promise;
    } else {
      // ui-router state transitions need to be wrapped in async functions when used inside route resolves, thus the $timeout
      $timeout(function() {
        $state.go('articles.list');
      });
    }
  }

Or do you think we should reconcile trailing slashes at the application ($stateProvider) level?

With this approach, we'd have to implement this on any routes that could have this potential issue. Then we'd have similar code throughout the router configurations. This might not be the most elegant solution. However, one benefit (other than it being very simple & low impact), is that we could put custom logic here to handle many different use cases; it ends up being very flexible.

@mleanos
Copy link
Member

mleanos commented Feb 28, 2016

@jloveland @vaucouleur I'd like to continue the discussion here, to find a good solution to this bug. I'm not super impressed with the solution that I've proposed in the PR.

See these comments for insight into what I've tried, and the issues I'm having..
angular-ui/ui-router#50 (comment)

See this branch for a better solution, that I can't seem to get working properly. Well, properly meaning I don't want to define a $urlRouterProvider.rule inside the Articles route configuration. I'd rather define it in the /core/client/app/init.js file, so we don't need to include it in every module.
mleanos@132d23a

Let me know what you think, or if you have any ideas. Maybe I'm doing something completely wrong. @vaucouleur Is this similar to the solution you mentioned above, that you tried?

@mleanos
Copy link
Member

mleanos commented Feb 29, 2016

Before a lot more work is done on this, I guess we should decide if we want to keep the trailing slashes, or remove them.

@ilanbiala
Copy link
Member

Usually trailing slashes aren't in the URL, so I think we should go that way to be consistent with most other sites.

mleanos added a commit to mleanos/mean that referenced this issue Mar 7, 2016
Adds an angular $urlRouterProvider service Rule to the Core module
configuration, that removes any trailing slashes in the URL for all routes.

The Rule is defined in the core routes configuration. Thus, in order for
this to work on all routes in the application, we have to inject the Core
module into each client module, as a dependecy in the client.module
configuration. Otherwise, we'd have to define the Rule in each module's route
configuration individually.

Adds missing client-side route configuration tests.

Tests demonstrate that the various route configurations can handle a trailing
slash in the URL, and gets resolved to the correct client route.

Fixes meanjs#1075
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants