When creating a route respect the default key that was being checked, also add a new key 'notFound' to create a NotFoundRoute

[Sean-Der]

Hey @natew,

First off, this is such an awesome project! Thank you so much for putting so much effort into it, it saved me a good week to just grab a setup and just go.

So, using reapp-routes I saw there was a check for route.default. I assume that at one time this was passed via opts? I also added support for notFound.

However, I don't think this is right since you should only be able to set one at time. Should I change this to be 'type' and the caller can pass a const? I am impartial how it is done, as long as the API allows it.

[natew]

Hey @Sean-Der, thank you. It's the results of months of trying to figure out how to piece it all together, so I'm happy people are using it.

I see what you mean. Perhaps @ryanflorence or @mjackson would have thoughts on the syntax:

now:

  route('app',
    route('page', { default: true})
    route('404', { notFound: true })
  )

potentially:

route('404', { type: route.default })

or (with more functions):

defaultRoute('404')

Also, should only be one notFound and only one Default (per-parent). We could handle this in the React way (detect if wrong programmatically and warn).

[Sean-Der]

Personally I like route('404', { type: route.DEFAULT }) But I might be biased (since that is the first thing that came to my mind). However, I am really not in tune with idiomatic ES6 package patterns just yet, so that might not be right.

Also, when I update the PR I will make sure to include warns! Thanks for grabbing this so fast.

[Sean-Der]

@natew what do you think of this syntax instead? The PR has been updated with this behavior

 import {notFoundRoute, defaultRoute} from 'reapp-routes';

  module.exports = ({ routes, route }) =>
    routes({dir: 'views'}, require,
      route('wrapper', '/', { dir: '' },
        route('home', {type: defaultRoute}),
        route('login'),
        route('404', {type: notFoundRoute})
      )
    );

[natew]

I like it but I have one worry. I'd want the syntax to not require a new import, which should work with your changes as is like this:

  module.exports = ({ routes, route, defaultRoute, notFoundRoute }) =>
    routes({dir: 'views'}, require,
      route('wrapper', '/', { dir: '' },
        route('home', {type: defaultRoute}),
        route('login'),
        route('404', {type: notFoundRoute})
      )
    );

Which to me is a little confusing, people may try using defaultRoute or notFoundRoute as functions. Which brings me back to the {type: route.default}. Or maybe:

  module.exports = ({ routes, route, type }) =>
    routes({dir: 'views'}, require,
      route('wrapper', '/', { dir: '' },
        route('home', {type: type.default}),
        route('login'),
        route('404', {type: type.notFound})
      )
    );

Another idea, since we are making this library automate and DRY things, is to infer automatically. Perhaps the first route in the list is the "default", and any route named "404" is automatically noFound. To me this verges on "too magical" though, but who knows.

@ryanflorence mentioned working on these features for react-router so would be good to get his opinion on it as well. Though I like that this library is decoupled from it in a way.

Another thing I need to do is get the syntax better for changing the default dir. To me the options on routes() should take require first, and the need for the first route to use { dir: '' } is unnecessary. But that's for another ticket.

[natew]

A final option would be to allow types directly, but this requires more argument shifting logic on our end (does typeof arg === 'Symbol' work with babel?):

  module.exports = ({ routes, route, type }) =>
    routes({dir: 'views'}, require,
      route('wrapper', '/', { dir: '' },
        route('home', type.default),
        route('login'),
        route('404', type.notFound)
      )
    );

[Sean-Der]

Oh I really like the argument shifting logic if we get typeof arg === 'Symbol' !

I agree with you on automatic route type setting being 'too magical' it would be really annoying to debug that if you didn't know about it. The documentation will still be pretty brisk if there was just a block that said 'how to set a route type, here are the types and their behaviors'

thanks for looking at this! and will watch out for @ryanflorence input, and update the PR as needed

[natew]

By the way I found a bug that was preventing the current method from working and pushed a fix today, so this should work:

route('name', { default: true })