isActive does not work for optional params

[pke]

According to the docs optional params are supported by isActive. However, it returns false for a location that is omitting the optional param.

Version

2.0.1

Test Case

 describe('with optional params', function () {
    it('is active when its optional params match', function (done) {
      render((
        <Router history={createHistory('/hello/ryan')}>
          <Route path="/hello(/:name)" />
        </Router>
      ), node, function () {
        expect(this.router.isActive('/hello/ryan')).toBe(true)
        expect(this.router.isActive('/hello')).toBe(true)
        expect(this.router.isActive('/hello/michael')).toBe(true)
        done()
      })
    })
  })

Expected Behavior

this.router.isActive('/hello')) should return true

Actual Behavior

this.router.isActive('/hello')) returns false

Or am I mis-reading the docs?

[evenstensberg]

@timdorr Maybe we could make optional params true by nature and throw new error if otherwise?
CreateTransitionManager used _isActive from 'isActive.js' and modifies states of location, routes and params. Maybe we could fix this there?

[timdorr]

@pke Added your test to the suite in #3284. Should be fixable 👍

[taion]

BTW, this is a duplicate of #3233, #3237, and many others.

[ryanflorence]

Michael and I have been talking about removing option params completely. Anything you can do with optional params can be done with component-less child routes, which I think is clearer to read and probably lets us simplify and speed up some code.

<Route path="foo/(:bar)" component={Thing}/>
<Route path="foo" component={Thing}>
  <Route path=":bar"/>
</Route>

Nothing would even change in Thing.

[timdorr]

I suppose the only thing I see is that it doesn't scream "this is optional!" to me. It certainly would make things a lot better internally, but it might make it more confusing for the end-user.

Who is using optional params anyways and what for? I get the sense it's a less used feature, so dropping the parens version of it would be low impact (negating the above argument).

[ryanflorence]

Yeah, it would be pretty low impact I think. Child routes are always optional, we're just not used to thinking about them that way :P Seems more obvious to me w/ no previous knowledge than our made up paren syntax.

[pke]

Optional params are helpful and clean and often used in server side backends for REST interfaces. So the syntax is not that uncommon. Uncommon is the component-less child route you propose where, at first glance, I'd have no idea where it ends and who is rendering what. Also, how would you describe a route with multiple optional params with that?

Anyway, optional params in one way or another should be addressed, because this here:

<Route path="cities"/>
<Route path="cities/:id"/>

also does not mark the active link correctly for the second route.

[ryanflorence]

Also, how would you describe a route with multiple optional params with that?

You keep nesting.

[pke]

@ryanflorence oh dear ;) Anyway the isActive matching is broken, even with the nesting approach isn't it?

[evenstensberg]

@pke The problem is the structure of the code. It needs to be pure, but still allow side-effects like optional routing, of which it might need a refactor.

[taion]

@pke

In your second example, right now, you should do:

<Route path="cities">
  <Route path=":id" />
</Route>

We agree that this is confusing, and we're thinking about updating the semantics of "active status" to just look at a path substring instead of looking at the route tree, which would satisfy your use case here.

We're currently tracking this on #3231; please follow along and comment if you're interested.

[taion]

Actually, sorry, this is the correct behavior. Nesting the routes some more is the right thing to do.

[robbinjanssen]

@ryanflorence @taion what about optionalParams which only apply to the indexRoute?

{
  path: 'projects(/:startDate/:endDate)',
  component: ProjectContainer,

  getIndexRoute(location, callback) {
    require.ensure([], (require) => {
      callback(null, [
        require('./List').default, // output: { getComponents: ... }
      ]);
    });
  },

  getChildRoutes(location, callback) {
    require.ensure([], (require) => {
      callback(null, [
        require('./Create').default, // output: { path: 'create', ... }
        require('./View').default,  // output: { path: 'view/:projectId', ... }
      ]);
    });
  },
};

props.router.isActive('/projects') does not match the indexRoute anymore when the optional params are provided.

What's the way to go here?

edit: never mind, solved it with adding the List as a childRoute as well, for those interested:

{
  path: 'projects',
  component: ProjectContainer,

  getIndexRoute(location, callback) {
    require.ensure([], (require) => {
      callback(null, [
        require('./List').default, // output: { getComponents: ... }
      ]);
    });
  },

  getChildRoutes(location, callback) {
    require.ensure([], (require) => {
      callback(null, [
        /* 
         * Note the `false` as param to the list below,
         * this appends the `path: '/:startDate/:endDate'` to the route object.
         */
        require('./List').default(false), // output: { path: '/:startDate/:endDate', ...}
        require('./Create').default, // output: { path: 'create', ... }
        require('./View').default,  // output: { path: 'view/:projectId', ... }
      ]);
    });
  },
};

[evanpurkhiser]

Maybe I'm missing something, but how can you structure routes to not require optional parameters when you have optional parameters at the beginning of the route?

For example I have

const catalogRoutes = [
  { path: ':guid', component: Catalog },
  { path: 'content/:guid', component: Content },
];

const routes = {
  path:      '/',
  component: App,

  childRoutes: [
    {
      path: '(chan/:channelCode/)catalog',
      indexRoute: { component: Catalog },
      childRoutes: catalogRoutes,
    },
  ],
};

I have a navigation link to /chan/myChannel/catalog. I would like that to be active when the active route is /catalog/myGuid or catalog/content/myGuid