RFC for router enhancements

[cibernox]

Expand Ember's router with support for optional segments and constraints.

Rendered

[nathanhammond]

No, they do not. They're literally skipped in the route parsing pass. The receiving handler should be capable of surviving params.optionalDynamicSegmentValue === undefined.

[nathanhammond]

The complexity has been reduced tremendously by matching segment-at-a-time.

[nathanhammond]

The paren would be inside of the segment: /(:region)/:productName

[cibernox]

In Rails the parenthesis enclose the slash too.
I don't have a strong opinion on this and if it's technically simpler so be it, but I think that it reads better that way because if you delete everything between parents you don't end up with a double slash.

[RuslanZavacky]

having (/:region) / inside also helps if your segment is the last one. In cases when you don't want your URL to be accessible by two different routes, eg. /my-cool-seo-url/ and /my-cool-seo-url. So you just specify, that additional segment only works, if there is a / in the end. Also, you might want to have the last segment as empty. And its possible to distinguish when you explicitly say, if there is / in the end, than segment should appear.

[RuslanZavacky]

And one more use-case is, that I actually want a lot :)
I have 3 languages -> en, es, fr
Default language is en. I should be able to have access to default route, without language prefix. Eg.

(/:lang)/welcome

Should be translated into:
/welcome
/es/welcome
/fr/welcome

[cibernox]

I think that the example I used is identical.

[RuslanZavacky]

Yeap, true :)

[nathanhammond]

Hey everybody! This isn't some greenfield proposal–I know exactly how to implement 99% of this and will have it working in an addon you can try in your applications in the next few days (which will also do build-time serialization as a side-effect). @cibernox has been really itching to have these features and wrote this RFC to get the discussion started.

[nathanhammond]

@RuslanZavacky @cibernox Y'all's thoughts on parentheses "including the slash" come from the (reasonable!) assumption that / is a meaningful character inside of the Router DSL. It isn't. Try it out!

Router.map(function() { 
  this.route('something', { path: '////something////'} );
});

{{#link-to 'something'}}Something{{/link-to}}

Output:

<a href="/something">Something</a>

But, if you move the parentheses outside of the segment definition you've now made it tremendously more difficult to parse. I'm standing firm on this one: the () are a property of the segment and belong between the slashes.

[RuslanZavacky]

@nathanhammond ah, so its not actually required to place it inside. If its easier to parse it, when its outside, I am quite okey with that, @cibernox thoughts? :)

[nathanhammond]

Regarding constraint functions: having thought about it more, I'm pretty sure that I want to avoid the situation where the constraint function has references to all sorts of interesting things that we may change to support tremendous performances wins (router precompilation). If we allow a user to define a function it's in a really dangerous scope for abuse. (See what I did inside of ember-route-alias. Hack and a half.)

I believe that for V1 we should only support regular expressions. Advanced users can still abuse that by throwing in an IIFE that returns a regular expression but we can throw an error at build time (via an AST analysis) to prevent that.

Edit: if we can guarantee it's a pure function via AST analysis then I'm okay with it, but in that scenario it should only receive the up-until-that-node parsed handlers and params as strings.

[nathanhammond]

This can also apply to glob segments.

Edit: Though it can be done, it'll be awkward to implement. Seems unlikely a glob segment will want to be constrained anyway. Let's not support it.

[cibernox]

I'm ok with constrain constraints (pun intended) to regexes initially if that simplifies the implementation. Support for functions could be added in the future if people really wants it. I'll remove references to function constrains from the RFC.

[cibernox]

About the parentheses wrapping the slash or not, I didn't suggested because I thought that slashes have a meaning in the routes, but for familiarity with rails. But if the parsing is significantly more complicated that way, let's make it simple.

[nathanhammond]

This would be sample4.opt because it has more handlers.

[IAmJulianAcosta]

@nathanhammond Hi! I just have a small question.

Currently I'm using dynamic segments like this:

/foo/xyz
/bar/xyz
/foobar/xyz

my router is:

this.route (
            'xyz',
            {
                path: ':param/xyz'
            }
        );

But I need param to be optional, so I can have a route /xyz. Did you think about cases like this one?

[nathanhammond]

@IAmJulianAcosta Order inside of the specified path doesn't matter, so this will work just fine. The microsyntax would be (:param)/xyz.

[IAmJulianAcosta]

@nathanhammond Thanks! I want to know also if the addon is ready to use or I'll have to wait a little bit for it. Thanks!

[jeffrey008]

Great to see this RFC. I have been using optional dynamic segments a lot in Rails, and I think we need one in Ember.
@nathanhammond I checked out your addon ember-route-alias, is your addon capable of doing optional dynamic segments at the moment?

Router.map(function() {
  this.route('sample', { path: '/foo/(:param)' });
});

Hope this RFC can be merged ASAP. Thanks the great work!

[simonihmig]

I have not seen a discussion about default values for optional segments here. Is that not under consideration?

Given the one example from the RFC wouldn't it be nice to do something like that:

Router.map(function() {
  this.route('forecast', {
    path: 'forecast/(:scale)',
    constraints: { scale: /(celsius|fahrenheit|kelvin)/ },
    defaults: { scale: 'celsius' }
  });
});


router.serialize('/forecast/celsius'); // => routeName='forecast', params: { scale: 'celsius' }
router.serialize('/forecast/newton'); // => not recognized route.
router.serialize('/forecast'); // => routeName='forecast', params: { scale: 'celsius' }

This way the forecast route would not have to explicitly handle the case where the 'scale' param would be undefined (see the example's last line).

[humanchimp]

@nathanhammond Did you end up making an add-on for this?

[simonihmig]

Just came back after needing this... Could we get a message (from core?) what the status is for this? I see a lot of support, no serious objections, is there something which prevents this from going into FCP?

[knownasilya]

Ping on the above 😄

[locks]

Why isn't constraints.scale an array of values?

[locks]

@simonihmig turning the question around, is there something preventing the community from implementing this via an addon?*

*To clarify, what I mean to ask is what's currently preventing people from doing that, is there a lower-level API we can provide that would aid in experimenting?

[simonihmig]

@locks tbh, I don't know at all. Regarding low-level APIs it seems @nathanhammond can probably answer this? I understood the purpose of this RFC to get this into core, but when there is a way to implement that as an addon without monkey patching things, I am fine with that.

Generally speaking, I would expect these RFCs to enter some defined state after discussion has settled. Like according to https://github.com/emberjs/rfcs#what-the-process-is to be either accepted and merged, or rejected and closed. But this seemed to be kind of left in a limbo, so I just wanted to ask what is preventing this to proceed in whatever way! :)

[mazondo]

Would this also allow us to define routes based on partial segments as well?

Specifically, looking to match
segment1/segment2-listing and segment1/segment2 to different routes

[erkie]

So much effort put into this RFC and now it's dormant... Or what's the status on this?

[nathanhammond]

Updates for those missing context:

• It's quite difficult to implement this in the existing route-recognizer.
• A fully API-compatible (as of whenever they diverged) fork of route-recognizer exists which I find to likely be a good starting point.
• I'm paid to not work on this. (Parse that carefully.)

If somebody wants to work on this, I believe the path forward to be:

• Fix a few minor issues in integration with Ember.
  • Make Ember's test suite pass after switching to the fork.
  • Write tests for the unspecified behavior which causes Ember's test suite to fail.
  • Prevent Ember from automatically generating an 'error' or 'loading' route if that route already exists.
• Reconcile changes since the fork.
  • Adopt any behavior changes.
  • Extract a shared test suite.
  • Possibly convert to TypeScript.
• Implement as a build-time patching addon so that people can opt-in to using it by just installing the addon.
• Implement optional segments in the fork.

[wagenet]

Given the inactivity here I'm going to go ahead and close this. I apologize for the fact that this stagnated and if you would still like to try to move this forward, I'll ensure that it gets a proper review.

It's also worth noting that there is some exploratory router work being done as part of Polaris. That may ultimately address these concerns.