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

Please add trailing slash support for can.route.pushstate #588

Closed
stevenvachon opened this Issue Dec 3, 2013 · 16 comments

Comments

Projects
None yet
3 participants
@stevenvachon
Contributor

stevenvachon commented Dec 3, 2013

No description provided.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 3, 2013

Contributor

Can you add a description / example? Why is trailing slash support needed? If you wanted to match everything under foo/, you can set the root to foo.

Contributor

justinbmeyer commented Dec 3, 2013

Can you add a description / example? Why is trailing slash support needed? If you wanted to match everything under foo/, you can set the root to foo.

@stevenvachon

This comment has been minimized.

Show comment
Hide comment
@stevenvachon

stevenvachon Dec 3, 2013

Contributor

Trailing slashes are HTTP spec for directories. It's common practice on marketing-type websites (blogs, etc) to simulate folders instead of resources. Would be a good idea to carry this mentality over to apps as they too rely on marketing tactics.

If we have an app at / and we want to support those with JavaScript disabled, we'd use something like NodeJS to serve them a generated HTML file using /index.html (and CanJS production files) when they try to access routes like /view1/sub-view/ that don't actually have a corresponding file/directory structure. When copying URLs between the JS and non-JS versions of the app, they should be able to work without automatic HTTPD redirects.

Contributor

stevenvachon commented Dec 3, 2013

Trailing slashes are HTTP spec for directories. It's common practice on marketing-type websites (blogs, etc) to simulate folders instead of resources. Would be a good idea to carry this mentality over to apps as they too rely on marketing tactics.

If we have an app at / and we want to support those with JavaScript disabled, we'd use something like NodeJS to serve them a generated HTML file using /index.html (and CanJS production files) when they try to access routes like /view1/sub-view/ that don't actually have a corresponding file/directory structure. When copying URLs between the JS and non-JS versions of the app, they should be able to work without automatic HTTPD redirects.

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Dec 3, 2013

Contributor

I can't find a reference for that use of trailing slashes in the HTTP spec. Do you have a link?
All that I can see is that a trailing slash should denote a folder on the webserver which doesn't really apply to pushstate. Escaping route/ to route%2F is a little weird but if you find this happened you could still just redirect to route.

Contributor

daffl commented Dec 3, 2013

I can't find a reference for that use of trailing slashes in the HTTP spec. Do you have a link?
All that I can see is that a trailing slash should denote a folder on the webserver which doesn't really apply to pushstate. Escaping route/ to route%2F is a little weird but if you find this happened you could still just redirect to route.

@stevenvachon

This comment has been minimized.

Show comment
Hide comment
@stevenvachon

stevenvachon Dec 3, 2013

Contributor

I edited my previous comment a few times to make it more accurate. Please check it out.

How does a virtual folder not apply to pushstate? We're already pointing to them, only difference is we're currently forced to end with a resource.

Contributor

stevenvachon commented Dec 3, 2013

I edited my previous comment a few times to make it more accurate. Please check it out.

How does a virtual folder not apply to pushstate? We're already pointing to them, only difference is we're currently forced to end with a resource.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 3, 2013

Contributor

I still don't really understand. Can you give me a code example or fiddle? Afaik, trailing slashes already work.

Contributor

justinbmeyer commented Dec 3, 2013

I still don't really understand. Can you give me a code example or fiddle? Afaik, trailing slashes already work.

@stevenvachon

This comment has been minimized.

Show comment
Hide comment
@stevenvachon

stevenvachon Dec 3, 2013

Contributor

This link ends up going to /test%2F:

can.route.bindings.pushstate.root = "/";
can.route(":section");
can.route.ready();
<a href="test/">link</a>

This ends up going to /test//:

can.route.bindings.pushstate.root = "/";
can.route(":section/:sub/");
can.route.ready();
can.route.attr({section:"test",sub:""});
Contributor

stevenvachon commented Dec 3, 2013

This link ends up going to /test%2F:

can.route.bindings.pushstate.root = "/";
can.route(":section");
can.route.ready();
<a href="test/">link</a>

This ends up going to /test//:

can.route.bindings.pushstate.root = "/";
can.route(":section/:sub/");
can.route.ready();
can.route.attr({section:"test",sub:""});
@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 3, 2013

Contributor

Could you change that to:

can.route.bindings.pushstate.root = "/";
can.route(":section/");
can.route.ready();

As far as the other example, that is what I would expect. You can probably fix that like:

can.route.bindings.pushstate.root = "/";
can.route(":section/",{
  sub: ""
});
can.route(":section/:sub/");
can.route.ready();
can.route.attr({section:"test",sub:""});
Contributor

justinbmeyer commented Dec 3, 2013

Could you change that to:

can.route.bindings.pushstate.root = "/";
can.route(":section/");
can.route.ready();

As far as the other example, that is what I would expect. You can probably fix that like:

can.route.bindings.pushstate.root = "/";
can.route(":section/",{
  sub: ""
});
can.route(":section/:sub/");
can.route.ready();
can.route.attr({section:"test",sub:""});
@stevenvachon

This comment has been minimized.

Show comment
Hide comment
@stevenvachon

stevenvachon Dec 4, 2013

Contributor

Changing to ":section/" resulted in my href="test/" link not dispatching a route event. A link with href="test" dispatches the event but then there's no trailing slash in the address bar.

Contributor

stevenvachon commented Dec 4, 2013

Changing to ":section/" resulted in my href="test/" link not dispatching a route event. A link with href="test" dispatches the event but then there's no trailing slash in the address bar.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 4, 2013

Contributor

Ok, so that's a bug. I'll try to fix this asap.

Contributor

justinbmeyer commented Dec 4, 2013

Ok, so that's a bug. I'll try to fix this asap.

@stevenvachon

This comment has been minimized.

Show comment
Hide comment
@stevenvachon

stevenvachon Dec 4, 2013

Contributor

Cool, thank you

Contributor

stevenvachon commented Dec 4, 2013

Cool, thank you

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 18, 2013

Contributor

Ok, I created a test that looks like:

    test("trailing slashes (#588)", function(){
        can.route.routes = {};
        can.route.bindings.pushstate.root = "/";

        can.route(":section/");
        res = can.route.param({section: "test"});

        equal(res, "test/");

        var obj = can.route.deparam("foo/");
        deepEqual(obj, {
            section : "foo",
            route: ":section/"
        });

    });

And it passes. I'm a bit confused. How are you creating the link?

Contributor

justinbmeyer commented Dec 18, 2013

Ok, I created a test that looks like:

    test("trailing slashes (#588)", function(){
        can.route.routes = {};
        can.route.bindings.pushstate.root = "/";

        can.route(":section/");
        res = can.route.param({section: "test"});

        equal(res, "test/");

        var obj = can.route.deparam("foo/");
        deepEqual(obj, {
            section : "foo",
            route: ":section/"
        });

    });

And it passes. I'm a bit confused. How are you creating the link?

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 18, 2013

Contributor

If I do:

<script>
    steal("can/route/pushstate", function(){
        can.route.bindings.pushstate.root = "/";
        can.route(":section/");
        can.route.ready();

        $(document.body).append("<br/>",can.route.link("foo",{
            section: "foo"
        }))

        can.route.bind("section", function(ev, newVal){
            console.log("section updated to",newVal)
        })
    })

</script>
<a href="/test/">test</a>

This calls back the section event handler. You'll notice that the link has to include the root's / like: <a href="/test/">test</a>. To make this easier, use can.route.link.

Contributor

justinbmeyer commented Dec 18, 2013

If I do:

<script>
    steal("can/route/pushstate", function(){
        can.route.bindings.pushstate.root = "/";
        can.route(":section/");
        can.route.ready();

        $(document.body).append("<br/>",can.route.link("foo",{
            section: "foo"
        }))

        can.route.bind("section", function(ev, newVal){
            console.log("section updated to",newVal)
        })
    })

</script>
<a href="/test/">test</a>

This calls back the section event handler. You'll notice that the link has to include the root's / like: <a href="/test/">test</a>. To make this easier, use can.route.link.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 18, 2013

Contributor

I'm going to close unless as there does not appear to be a bug. Please comment if I'm missing something.

Contributor

justinbmeyer commented Dec 18, 2013

I'm going to close unless as there does not appear to be a bug. Please comment if I'm missing something.

@stevenvachon

This comment has been minimized.

Show comment
Hide comment
@stevenvachon

stevenvachon Dec 18, 2013

Contributor
can.route.bindings.pushstate.root = "/";
can.route(":section/");
can.route.ready();

can.route.attr({section: "test"});

goes to /test, not /test/ on my machine

and

can.route.bindings.pushstate.root = "/";
can.route(":section/");
can.route.ready();

console.log( can.route.url({section: "test"}) );

gives me /test, not /test/

am I doing something incorrectly?

Contributor

stevenvachon commented Dec 18, 2013

can.route.bindings.pushstate.root = "/";
can.route(":section/");
can.route.ready();

can.route.attr({section: "test"});

goes to /test, not /test/ on my machine

and

can.route.bindings.pushstate.root = "/";
can.route(":section/");
can.route.ready();

console.log( can.route.url({section: "test"}) );

gives me /test, not /test/

am I doing something incorrectly?

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 19, 2013

Contributor

@stevenvachon I'm not sure. Let chat on skype tomorrow: justinbmeyer.

Contributor

justinbmeyer commented Dec 19, 2013

@stevenvachon I'm not sure. Let chat on skype tomorrow: justinbmeyer.

@justinbmeyer justinbmeyer reopened this Dec 19, 2013

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 19, 2013

Contributor

This is confirmed working with @stevenvachon

Contributor

justinbmeyer commented Dec 19, 2013

This is confirmed working with @stevenvachon

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