Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Counterintuitive behavior with trailing slashes in patterns #37

Open
rictic opened this issue Mar 8, 2016 · 14 comments
Open

Counterintuitive behavior with trailing slashes in patterns #37

rictic opened this issue Mar 8, 2016 · 14 comments

Comments

@rictic
Copy link
Collaborator

rictic commented Mar 8, 2016

A trailing slash in a pattern currently means that you want to match a slash followed by the empty string.

This is counterintuitive, and an easy place to make mistakes. However, it's also useful, for specifically matching paths that end with a trailing slash.

@rictic
Copy link
Collaborator Author

rictic commented Mar 8, 2016

For now I'm explaining the existing behavior in the article and commenting that this is something that we're actively considering.

@tjsavage
Copy link

Ran into this, and also the inverse - if you don't have a trailing slash on the URL, the carbon-route with a leading slash in the pattern won't match it. Agreed - for now it's a good thing to make sure to call out in the documentation.

@rictic
Copy link
Collaborator Author

rictic commented Apr 1, 2016

As this is an API feedback bug I'd love to hear more from folks who are running into this.

Anyone running into this? If so, was it something you noticed and got past quickly or did it leave you scratching your head for a while. Let us know!

@TimvdLippe
Copy link

While not specifically trailing slashes, I am currently having issues with matching the root directory. My setup is as follows:

<carbon-location route="{{route}}"></carbon-location>
<carbon-route route="{{route}}" pattern="/:page" data="{{routeData}}"></carbon-route>
<iron-lazy-pages selected="{{routeData.page}}" attr-for-selected="data-route">
  <template is="iron-lazy-page" data-route="">
    Home
     <a href="/store">Visit the store.</a>
  </template>
  <template is="iron-lazy-page" data-route="store" path="store-page/store-page.html">
    This is the store page.
     <a href="/">Head back home.</a>
  </template>
</iron-lazy-pages>

Sadly this is not working, because /:page does not match /. Debugging the route results in:

Object {prefix: "", path: "/", __queryParams: Object}

whereas data results in

Object {page: null}

What I would expect (and to make a home-page routing work) is that the data.page is "" instead of null.

@TimvdLippe
Copy link

To answer the question: so far I have not yet find a solution to have a homepage matching root. E.g. if in the above example I visit the store and click on Head back home. the router stops working and I see a white screen. Since having a home-page is crucial (:stuck_out_tongue:) and more intuitive, I would like to see this getting fixed.

EDIT: Okay, the above is not true. I had to make the binding with iron-lazy-pages.selected a one-way binding and then it was working. The issue was that it somehow fired observers of routeData. This triggered __updatePathOnDataChange which set this._skipMatch to true and therefore the next call to __tryToMatch was ignored. As a result, the "Head back home" did not result in a trigger on carbon-route and not switching to the correct page.
In the end it cost me quite some trouble to figure out what the problem was, so it would be nice if the issue described in my previous comment could be fixed.

EDIT2: Sigh it was a problem with iron-lazy-pages which incorrectly set selected to null. This locked the carbon-route from updating and entered an invalid state.

@timeu
Copy link

timeu commented Apr 12, 2016

I already gave an example in the linked issue #75, but currently it is not possible to navigate from a parent route (i.e. domain.com/users) to a subroute (i.e. domain/users/1) purely with databinding, because of the missing trailing slash.
@TimvdLippe PR #77 seems to fix this (haven't tested it yet tough).

@mgibas
Copy link

mgibas commented Apr 21, 2016

Im running into same issue that @timeu mentioned.

@akc42
Copy link
Contributor

akc42 commented Jun 30, 2016

As referenced above I raised this in issue #123. The real problem is not, to my mind related to trailing slashes or not, it is that fact that stale data is left in tail. I have several layers of iron-pages selecting between different things, and at each layer down the hierarchy I really want route.page to be passed to my lazy loading behaviour such that I can go this.page = page || this.homePage();

the homePage function is abstract in my behaviour, and at each level I import it I can use it to set a page to handle the most selections and a null selection from home page. Generally homePage returns 'menu', so I can always simulate it by using a url of /menu or /reports/menu or /reports/bydate/menu etc.

I have found the best way for deep down layers of my application to jump to a new path is to include an iron location element and a 'path' property. If I then do <iron-location path="{{path}}"></iron-location> somewhere in my template I can jump to any url with a simple this.path ='/new/url'.

Unfortunately to make this work the way it is currently set up means I have to use these strange '/reports/menu' which get echoed to the browser location bar. I doesn't matter too much to the application (as long as I know the default) but isn't really what I want to show my user.

I haven't got as far as trying to handle something like referenced in #75, but the principal will be the same. Instead of /user being a user selection page and /user/4 being the page showing the details of user 4, I would have to invent a url like /user/0 to support the page selection with iron-pages. Not something that makes sense to me.

@mercmobily
Copy link

@akc42 I am having the exact same issue: stale paths are left over in the route's tail because there is no match without the trailing slash...

@vinerz
Copy link

vinerz commented Jan 18, 2017

As @rictic asked, I'm about two hours already trying to figure out how to deal with the differences between

/forgot
and
/forgot/

That's because /forgot has its internal router getting the tail of the root element, and even if I force the URL update by firing a this.set('routeData.subView', 'begin');, which is the default and home page of this element, I am not able to bubble it to the root. It only works if I start the app using the trailing slash.

The first one, that I am using throughout the app, doesn't work, because the router never gets active.

I don't know what to do anymore.

@ronnyroeller
Copy link

ronnyroeller commented Feb 3, 2017

I've a similar issue: I want to make tab optional in this pattern /:id/:tab

Only option that I could find so far:

<app-route route="{{route}}" pattern="/:id" data="{{_idData}}" tail="{{_idTail}}"></app-route>
<app-route route="{{_idTail}}" pattern="/:tab" data="{{_tabData}}"></app-route>

@NeverOddOrEven
Copy link

For anyone searching for a quick fix, I hope this helps: https://jsbin.com/meduqu/20/edit?html,output. Lines 17 and 47 are the main things to look at.

Line 17:

<app-location route="{{window_location}}"></app-location> 
<app-route route="{{window_location}}" pattern="/:id" data="{{a}}" tail="{{foo_subroute}}"></app-route>
<app-route route="{{foo_subroute}}" pattern="/:id" data="{{b}}" tail="{{bar_subroute}}"></app-route>

Line 47:

observers: [
  'selected(a.id)',
  'selected(b.id)'
],
selected: function(newVal) {
  if (this.window_location.path.slice(-1) !== '/') {
    this.set('window_location.path', this.window_location.path + '/');
  }
}

@bennypowers
Copy link

The problem with @NeverOddOrEven's solution is that it breaks the back button. The user will now have to press back twice.

@pedro2555
Copy link

pedro2555 commented Mar 16, 2020

I've managed to compile a good, from my perspective anyway, solution to this problem.

The trick is to leverage the active property combined with the data one.

The problem

Routes

We have two levels of routing that configure the following possible urls:

  • /items: show, in a list, all items
  • /items/:id: show the item with :id details
  • /items/:id/:detail: show the item with :id the specific :detail

Trailing slashes must be dealt with in the same way as no trailing slashes (/items and /items/ must be equal).

Where :detail can technically be any element, if we don't have that page we show a 404. But other solutions add no complexity.

The solution

The 404 fallback is omitted from this example for the sake of simplicity.

Elements

All visible pages are controlled using iron-page elements and its page attribute.

An -app element, identical to the starter kit, controls the /items route and displays a -items-page child element. The app-route tail property is propagated to -items-page.

items-page element

<app-route
    data="{{routeData}}"
    pattern="/:id"
    route="[[route]]"
    tail="{{subRoute}}"
></app-route>

<iron-pages
    attr-for-selected="name"
    fallback-selection="list"
    role="main"
    selected="[[page]]"
>
    <list-view name="list"></list-view>
    <item-view name="item" route="[[subRoute]]"></item-view>
</iron-pages>

static get observers() {
    return [
        '_routeIdChanged(routeData.id)'
    ];
}

_routeIdChanged(id) {
    let page = id || 'list';

    this.page = page == 'list' ? page : 'item';
}

list-view element

List view deals with no routes, is a simple view.

item-view element

Item view deals with a series of different views, depending on the url, defaults to a primary view.

The trick for this to work, is to compute the page to display using not only the data attribute, but also the active attribute. The last one will trigger any observers even when no slug is provided, allowing us to decide if and how to act.

<app-route
    active="{{routeActive}}"
    pattern="/:slug"
    route="[[route]]"
    data="{{routeData}}"
></app-route>

<iron-pages
    attr-for-selected="name"
    fallback-selection="primary"
    role="main"
    selected="[[page]]"
>
    <item-primary-view   name="primary"></item-primary-view>
    <item-secondary-view name="secondary"></item-secondary-view>
</iron-pages>

static get properties() {
    return {
        route: {
            type: Object,
            reflectToAttribute: true,
        },
        page: {
            type: String,
            computed: '_computePage(routeActive, routeData)',
        },
    };
}

_computePage(routeActive, routeData) {
    return routeActive ? routeData.slug : 'primary';
}

Conclusion

I see no reason for an API change. But rather a need for properly documenting these kinds of patterns. This is a flexible, scaleable, and coherent solution. And nothing is specific to trailing slashes, meaning there is no code that specifically considers it. Trailing slashes seem to be a symptom of improper patterns.

API changes will only result in a more magical solution. Which may create a good first impression, but ultimately result in a less coherent behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests