Skip to content

Redirects - #62

Closed
jamiebuilds wants to merge 1 commit into
emberjs:masterfrom
jamiebuilds:redirects
Closed

Redirects#62
jamiebuilds wants to merge 1 commit into
emberjs:masterfrom
jamiebuilds:redirects

Conversation

@jamiebuilds

Copy link
Copy Markdown

@jamiebuilds
jamiebuilds force-pushed the redirects branch 4 times, most recently from 01f1c7e to 61a52d6 Compare June 6, 2015 23:52
@workmanw

workmanw commented Jun 7, 2015

Copy link
Copy Markdown

This is an interesting RFC. I would imagine many people have this use case.

We too had this problem, instead of creating routes that redirected on beforeModel, we choose to use App.deferReadiness(), then ran the url through a basic redirect map we built, updated the window.location if necessary, then called App.advanceReadiness(). We felt this pattern kept all the redirect logic separate from the route system it's self.

I'd probably use this if implemented. Anyways, just my $0.02.

@wycats

wycats commented Jun 7, 2015

Copy link
Copy Markdown
Member

@thejameskyle This is a pretty good direction, but I have a few concerns:

  1. The "from" approach doesn't feel that good when more complex routes are involved. Something feels weird about embedding the String DSL on the right hand side.
  2. Have you considered having these semantics generate a route, so that more involved use-cases (destinations based on the values of dynamic segments or *path segments) could be handled directly?

@tikotzky

tikotzky commented Jun 7, 2015

Copy link
Copy Markdown

has doing something like this.redirect('foo', 'bar') been considered?

That's similiar to how react router does it.
See http://rackt.github.io/react-router/#Redirect

@kimroen

kimroen commented Jun 7, 2015

Copy link
Copy Markdown

@tikotzky Yes, it's mentioned under "Alternatives".

@MiguelMadero

Copy link
Copy Markdown

@kimroen I think that the redirect alternative has some benefits. It will help with nesting, you can have a wildcard for redirects (e.g. this.redirect('foo/*, 'bar/*')), the router already takes care of the duplicates for this cases and also, the implementation could be as simple as Ember creating a default route that knows how to redirect, that could be overridden for more complex scenarios, e.g. maybe we need to load a model, find some parameter from there and then redirect. It also makes the transition form an old to a new route a bit simpler, since we could leave the old route definition in place and simply rename to redirect.
I agree with you that the other approach is simpler to maintain, but I would expect this not to need a lot of maintenance, since it's simply a "legacy" route that we want to maintain in case someone bookmarked it, right?

@jamiebuilds

Copy link
Copy Markdown
Author

The "from" approach doesn't feel that good when more complex routes are involved. Something feels weird about embedding the String DSL on the right hand side.

Can I get a couple examples of really complex routes that I could play around with? Most of the ones I work with are fairly simple.

As for the redirectFrom string DSL, I think it would end up in any implementation here. Luckily everything is already there from the normal path string DSL.

Have you considered having these semantics generate a route, so that more involved use-cases (destinations based on the values of dynamic segments or *path segments) could be handled directly?

I wasn't sure about the exact implementation semantics, however I was imagining that these would generate routes that would be functionally equivalent to how redirects are handled today.

I wonder if a similar approach can be taken with how other routes are generated: Only generate a Route when it does not exist.

@tikotzky

tikotzky commented Jun 7, 2015

Copy link
Copy Markdown

@kimroen sorry I missed that.

@MiguelMadero I agree, in my experience setting up a redirect is a one time thing that you do at the time you create the new route.
If you later change the route that you are redirecting to then you have a choice of either editing the existing redirect or adding a new redirect which would cause a user with the oldest link to get redirected twice.

For example with

this.route('foo');
this.redirect('bar', 'foo');

if you later decided to change foo to baz you would do

this.route('baz');
this.redirect('bar', 'foo');
this.redirect('foo', 'baz');

if a user then visited /bar they would get redirected to /foo which would redirect to /baz

@jamiebuilds

Copy link
Copy Markdown
Author

I thought about pushing this down to the destination route object itself where you could create an api that has a bit more freedom to manipulate the url.

Route.extend({
  redirectFrom: ['bar', 'baz', {
    'foo/:id': (params) {
      return params;
    }
  }]
});

Not sure about what the api would be here, but it seemed very odd to push the url down to this point.

chancancode pushed a commit that referenced this pull request Oct 27, 2017
Fixes #62. Of the formats proposed there, I went with option (e.c).
@locks locks added the T-routing label Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants