remix-run / react-router

Declarative routing for React
https://reactrouter.com
MIT License
53.1k stars 10.31k forks source link

path={fn} #221

Closed brigand closed 10 years ago

brigand commented 10 years ago

In #142 there is discussion of making route matching more powerful, but still very convenient. This is great, but I think it's very important to provide a fully powered alternative as a catch-all for things that the convenience syntax doesn't cover. Even adding regex support would be only half solving the limitation.

Here's an example:

<Routes>
  <Route handler={Users} >
    <Route name="user-single" prefix="/users" path={fn} handler={UserView} />
    <Route name="user-not-found" path="/users/*" handler={UserNotFound} />
  </Route>
</Routes>

If the url is "/users/10034566", the function should be called like so:

var route = {
  path: "10034566",
  fullPath: "/users/10034566",
  parts: ["users", "10034566"]
};
var callback = function(err, result){};
fn(route, callback);

This is nice and powerful, but we still want to keep the convenience, so either in react-router, or a separate package, there should be utilities such as a regex utility:

function regexPath(regex, info, callback) {
  if (arguments.length === 1) {
    // partial application
    return regexPath.bind(null, regex);
  }

  var match = info.path.match(regex);
  if (match) {
    callback(null, {match: match});
  }
  else {
    callback({type: "RegexMatch", message: "The url " + info.fullPath + " is invalid"});
  }
}

If our function gives an error, the next matching path receives an error property; otherwise it receives a success (name ideas?) property.

All of the complexity and power of this can be a page in the docs, pointed to by a link saying "Need more control of matching?".


My first use case of this is I have routes where I need to match UUIDs, which can be done with regexp, but I'd like to abstract this into a uuid helper in my own code. If we have this, when someone has a weird convenience request, you can give a solution they can implement right away while considering the other option.

Note: the asynchronous api might be going to far for this. Perhaps returning an object with an error or success property would be sufficient.

gaearon commented 10 years ago

+1, I'd like to validate my routes with a function

ryanflorence commented 10 years ago

What's wrong with validating on your component's willTransitionTo?

ryanflorence commented 10 years ago
var User = React.createClass({
  statics: {
    willTransitionTo: function(transition, params) {
      if (!validate(transition.path)) {
        transition.redirect('user-not-found');
      }

      // or
      return validateAsync(transition.path).then(null, function() {
        transition.redirect('user-not-found');      
      });
    }
  }
});
ryanflorence commented 10 years ago
var ValidateUUID = function(param, redirectTo) {
  return {
    statics: {
      willTransitionTo: function(transition, params) {
        if (!validate(params[params])) {
          transition.redirect(redirectTo);
        }

        // or
        return validateAsync(params[param]).then(null, function() {
          transition.redirect(redirectTo);
        });
      }
    }
  }
};

var User = React.createClass({
  mixins: [ValidateUUID('userId', 'user-not-found')]
});

var Account = React.createClass({
  mixins: [ValidateUUID('accountId', 'account-not-found')]
});
gaearon commented 10 years ago

I didn't realize I could use this. It definitely fits my use case, thanks.

brigand commented 10 years ago

@rpflorence willTransitionTo implies the route is acceptable. Instead of skipping that route as if it didn't match, it would trigger the route and then I redirect elsewhere; which couples my route ordering and willTransitionTo implementation, so I must maintain the same logic in a different format, in two places.

I need to essentially figure out what the correct route is myself, instead of leveraging the router.

Other than the extra complexity, willTransitionTo does solve the problem. If you don't think it would be good to have, feel free to close the issue.

ryanflorence commented 10 years ago

I think I get it now. I find the idea intriguing :) I'm disinclined to make it async though.

simenbrekken commented 10 years ago

+1 I'm having the exact same issue.

mjackson commented 10 years ago

If we do this, I'd like to call it <Route test>. The default test will just be to see if the path matches, like it does now. You can get as fancy as you want inside your test function. It should not be async.

But what I'm not seeing here is real world use cases. In my experience a RegExp is almost always over-engineering the problem. It's a hammer and every problem is a nail.

Can we pin down some solid use cases where the extra path matching behavior described in #142 (along with DefaultRoute and NotFoundRoute) is not sufficient?

ryanflorence commented 10 years ago

I'm also interested in real world use-cases. One of the appeals of this router is being able to look at the route config and understand a whole lot about the app. If we give people a function to test the routes then you lose all of that.

simenbrekken commented 10 years ago

As I mentioned in my comment in #142 I'm using RegExp to match URLs where the the contents of one or more URL segments dictates if a route matches or not.

With the suggested <Route test> from @mjackson I could ditch the RegExp matching in favor of a more verbose, easier to read and less error prone matching function.

ryanflorence commented 10 years ago

@sbrekken I don't mean to be a pest, but you didn't explain a use-case, you explained an implementation of a use-case. What is the non-programmer explanation of what is going on in your app? I really want to understand.

simenbrekken commented 10 years ago

@rpflorence The user browses products in a store. http://store.com/clothing/jackets/winter would show the winter jacket category and http://store.com/clothing/jackets/winter/warm-jacket-123/blue-large-456 would show the product page for a warm blue winter jacket in size large.

Similarly I could find the same jacket in the winter sales category if i visit http://store.com/winter-sale/jackets/winter/warm-jacket-123.

To summarize: Depending on contents and quantity, URL segments are either categories, products or product variants (color & size).

ryanflorence commented 10 years ago

Seems like you can define routes for each url scheme and give them the same handler, like Product below?

<Route path="/:category" handler={Category}>
  <DefaultRoute handler={CategoryIndex}/>
  <Route path="/:category/:season/:type" handler={ProductType} />
  <Route path="/:category/:season/:type/:product" handler={Product} />
</Route>
<Route path="/:promotion/:season/:type/:product" handler={Product} />
simenbrekken commented 10 years ago

@rpflorence URL depths are completely dynamic, you can reach the same jacket from all the following URLs:

Likewise for a specific size:

The reason for this is that a product isn't contained within a category. A category merely defines a set of conditions (e.g. product must be tagged with "winter" and "jacket") which it uses to filter products, similar to what Amazon and others do.

Category segments in the URL are basically only used to establish where in the hierarchy you currently are. If you started browsing from http://store.com/winter-sale/jackets/warm-jacket-123, the product page would include a breadcrumb (winter-sale » jackets) that would let you browse other jackets in the winter sale.

ryanflorence commented 10 years ago

If there is a breadcrumb then there is some set of known hierarchy, unless the breadcrumb is browsing history.

But okay, lets say I on board with this path={fn}:

How do you make a link to a product? <Link/> needs to take params and fill in a path defined on a route.

simenbrekken commented 10 years ago

@rpflorence Indeed there is a known hiearachy but it's again completely dynamic. Currently I attempt to find the category by parsing the URL segments and redirect you to a 404 if it wasn't found.

Linking to products can be done in two ways, either by the standard <Link to="product" id="123" /> or by constructing an URL using an internal helper function. (URL.toProduct(categoryAncestors, product) => /winter-sale/jackets/warm-jacket-123/first-size-color-combo-456)

simenbrekken commented 10 years ago

@rpflorence As far as I can see, combining test and willTransitionTo will solve this. Firstly locate handler for the route using test and then ensure data for the route actually exists with willTransitionTo.

mjackson commented 10 years ago

@sbrekken FWIW willTransitionTo feels like the wrong place to check for the existence of data. Transition hooks are async, but they also block the transition from happening. So anything inside a transition hook needs to happen as quickly as possible. I don't know how your code is structured, but doing stuff like XHR inside transition hooks should be avoided.

ryanflorence commented 10 years ago

@sbrekken does the order of your segments matter?

simenbrekken commented 10 years ago

@mjackson You might be right but I'd rather block the transition for what in my case is ~100ms than render a page that will always be empty. I'm more than open for suggestions on how to work around this.

It should be noted that this commerce app also renders on the server.

simenbrekken commented 10 years ago

@rpflorence Currently yes, because the product and variant (size-color combination) patterns are essentially the same. I can easily avoid this since I'm really just looking for the product, the following variant pattern only locates what size-color combination to show initially.

ryanflorence commented 10 years ago

Do the urls render different UI around the product? Or are they all simply vanity urls?

simenbrekken commented 10 years ago

@rpflorence They're only vanity and they are matched last.

Routing config for good measure:

<Routes location="history">
  <Route name="frontpage" path="/" handler={Frontpage} />
  <Route name="checkout" path="checkout" handler={Checkout} />
  <Route name="category" horriblyComplicatedRegExp={categoryPattern} handler={Category} />
  <Route name="product" ridiculouslyConvolutedRegExp={productPattern} handler={Product} />
  <Route name="not-found" handler={NotFound} />
</Routes>
ryanflorence commented 10 years ago

Hmm ... so why have them? to construct the breadcrumb?

ryanflorence commented 10 years ago

(thanks for your patience, btw, I'm just trying to know enough to have a good reason to either support this feature or not)

simenbrekken commented 10 years ago

@rpflorence Yes, the navigation tree is traversed by segment resulting in the breadcrumb and child category links.

ryanflorence commented 10 years ago

If you could match the path with a function, you'd still need your regex in your handler to parse out the data you need for the breadcrumb. confirm/deny?

simenbrekken commented 10 years ago

@rpflorence Confirmed, I'm using the last part of each segment to identify the resource, e.g /shoes/trainers/marty-mcfly-power-lace-123/blue-suede-456.

ryanflorence commented 10 years ago

I guess my question then is why would you use <Route test/> when you have to do the same work anyway?

simenbrekken commented 10 years ago

@rpflorence I'm probably single-tracked here, do you have a different solution? I need to differentiate /clothing/shoes/trainers (Category) from /clothing/shoes/boots-made-for-walken-123 (Product).

ryanflorence commented 10 years ago

I'm assuming all of your categories are data driven so you can't declare them up front.

<Routes>
  <!-- all of your non-dynamic routes -->
  <Route name="company" handler={Company} />
  <Route name="contact" handler={Contact} />
  <!-- etc -->

  <Route name="product" path="*" handler={ProductHandler} />
</Routes>

Now Product will catch everything else that you don't recognize. Validate it, if invalid, redirect somewhere, if valid, render the product page with the breadcrumb data.

var ProductHandler = React.createClass({
  statics: {
    willTransitionTo: function(transition, params) {
      if (!validProductPath(params.splat))
        transition.redirect('not-found');
    }
  },

  getInitialState: function() {
    return {
      breadcrumbData: parseProductPath(this.params.splat)
    };
  },

  render: function() {
    return <Product breadcrumbData={this.state.breadcrumbData} />
  }
});

It would probably be productData and that would contain breadcrumb and product data ... but this illustrates the idea.

simenbrekken commented 10 years ago

@rpflorence Thanks for the example solution. I've expanded it a bit and discovered a few shortcomings.

Without test I don't know which handler to invoke, everything has to be rendered through Products. This means I can't use different handlers for an a Product or a Category of products.

I also can't leverage AsyncState on the server without duplicating the splat parsing in getInitialStateAsync to fetch the correct data for the matching handler. I also probably have to duplicate it in componentWillReceiveProps as well.

var Products = React.createClass({
  render: function() {
    var categories = [];
    var productId;
    var variantEan;

    var pattern = /-([0-9]+)$/;
    var segments = this.props.params.splat.split('/');
    segments.some(function(segment) {
      var matches = pattern.exec(segment);

      if (matches) {
        var id = Number(matches[1]);

        if (productId) {
          variantEan = id;
        } else {
          productId = id;
        }
      } else if (productId) {
        return true;
      }

      categories.push(segment);
    });

    var content;

    if (productId) {
      content = <Product categories={categories} productId={productId} variantEan={variantEan} />;
    } else {
      content = <Category categories={categories} />;
    }

    return (
      <div>
        Products

        {content}
      </div>
    );
  }
});
<Routes location="history">
  <Route path="/" name="frontpage" handler={Frontpage} />
  <Route name="checkout" handler={Checkout} />
  <Route name="products" path="*" handler={Products} />
</Routes>
mjackson commented 10 years ago

@sbrekken This seems like a fine way to do it. The common logic can be abstracted out into its own function that parses the splat, saving the hassle of literally duplicating the code.

But you're correct that everything needs to go through Products given your URL structure, which means that you can't use separate route handlers for Product and Category. Is that a problem? i.e. do you need to do different stuff in your transition hooks between those two handlers?

simenbrekken commented 10 years ago

@mjackson Not that I know of yet, but they're fundamentally different pages that deal with different domains. One lists, filters, sorts and products. The other views details about a product, switches between sizes and colors, adds products to a cart etc.

I'd much rather have them separate, the approach above feels like a workaround.

mjackson commented 10 years ago

@sbrekken They are still separate. They are completely separate components, not routes. Route handler components can do all of the rendering logic 99% of the time. But every once in a while it makes sense to have a route handler component that is used purely for route handling and delegates rendering to other components.

simenbrekken commented 10 years ago

@mjackson I still think test would solve this more elegantly as a simple function that checks if the route should match or not (even if the path matches):

<Routes location="history">
  <Route path="/" name="frontpage" handler={Frontpage} />
  <Route name="checkout" handler={Checkout} />
  <Route name="category" path="*" test={isValidProductPath) handler={Product} />
  <Route name="product" path="*" test={isValidCategoryPath} handler={Category} />
  <NotFoundRoute handler={NotFound} />
</Routes>

Here isValidCategoryPath could easily check if there really is a category with the given path.

ryanflorence commented 10 years ago

or just return a <Category/> or <Product/> in your handler, I don't see how test makes the code objectively better.

simenbrekken commented 10 years ago

@rpflorence I guess I can live with the example above with some cleanup. Maybe we can close this one for now and just return to it later if needed?

mjackson commented 10 years ago

@sbrekken Adding test certainly makes the route config a little more obvious. But now:

I'm going to close this for now. We can always reopen later if someone wants to make an argument for test.

simenbrekken commented 10 years ago

I thought I'd just leave our current solution here which doesn't require any manual parsing:

var routes = (
  <Routes location="history">
    <Route path="/" handler={Application}>
      <Route name="frontpage" path="/" handler={Frontpage} />
      <Route name="not-found" handler={NotFound} />
      <Route name="product" path="*/:productSlug,:product/:variantSlug,:variant" handler={Product} />
      <Route name="category" path="*" handler={Category} />
    </Route>
  </Routes>
)

The only unsolved issue is that we have to redirect to not-found instead of displaying a "Not found" page with current URL.