iron-meteor / iron-router

A client and server side router designed specifically for Meteor.
MIT License
1.98k stars 413 forks source link

Router.go stopped working with parameters #1572

Closed evolross closed 7 years ago

evolross commented 7 years ago

My app is on Meteor 1.1.0.3 and probably an older version of iron:router. Something that just started happening in my app as of a few days (possibly weeks) ago, calling:

Router.go('productView', {productCode: productCode})

Returns the following error:

Exception in delivering result of invoking 'checkProductCode': TypeError: path.replace is not a function
at Url.resolve (http://192.168.0.6:3002/packages/iron_url.js?6bcdffd55f21f5f8c3a0bcaf3998acda7ab161c3:548:8)
at Handler.resolve (http://192.168.0.6:3002/packages/iron_middleware-stack.js?7f82b91756b9a8653df74ac35a85de10b7b68ff7:125:27)
at Function.Route.path (http://192.168.0.6:3002/packages/iron_router.js?7b16fe70bccff9182aca02afb2ccd7708ac57c64:861:23)
at Function.Router.go (http://192.168.0.6:3002/packages/iron_router.js?7b16fe70bccff9182aca02afb2ccd7708ac57c64:1836:18)
at http://192.168.0.6:3002/client/templates/products/product_view_gateway.js?d917f4d066bef84ed57134f49d2850855a7d00cd:51:20
at MethodInvoker._callback (http://192.168.0.6:3002/packages/meteor.js?43b7958c1598803e94014f27f5f622b0bddc0aaf:983:22)
at MethodInvoker._maybeInvokeCallback (http://192.168.0.6:3002/packages/ddp.js?d1840d3ba04c65ffade261f362e26699b7509706:3860:12)
at MethodInvoker.receiveResult (http://192.168.0.6:3002/packages/ddp.js?d1840d3ba04c65ffade261f362e26699b7509706:3880:10)
at Connection._livedata_result (http://192.168.0.6:3002/packages/ddp.js?d1840d3ba04c65ffade261f362e26699b7509706:4970:9)
at onMessage (http://192.168.0.6:3002/packages/ddp.js?d1840d3ba04c65ffade261f362e26699b7509706:3725:12)

The route looks like this:

this.route('productView', {
   path: ['/:productCode']
   ...

Now, the only way that's working is to make the call like this:

Router.go('/' + productCode);

Just curious if anyone was 1. still using iron:router and 2. running into this?

chrisbutler commented 7 years ago

@evolross can you tell me what version of iron router you are using?

evolross commented 7 years ago

These are the package versions:

iron:controller@1.0.8
iron:core@1.0.11
iron:dynamic-template@1.0.8
iron:layout@1.0.8
iron:location@1.0.9
iron:middleware-stack@1.1.0
iron:router@1.0.9
iron:url@1.0.11

I looked at these and these haven't changed in my app for months because I've been on the same version of Meteor (1.1.0.3) for a long time. So not sure what happened.

chrisbutler commented 7 years ago

yeah that is very strange. if nothing has changed with anything iron related, it seems like the problem must be elsewhere, but i'll see what i can come up with. closing this for now

evolross commented 7 years ago

I figured out what the issue is. It may still be a bug. It has to do with me defining a route with this pattern:

this.route('productView', {
   path: ['/', '/:productCode'],                
   waitOn: function() {
      if(this.params.productCode) { 
         return Meteor.subscribe('productByCode', this.params.productCode);
      }
     else
         return Meteor.subscribe('productByCode', '123456789');
   }
   ...

I was trying to define a route who's path could be / or /:productCode and if the route was / it would subscribe to a default product (e.g. 123456789) using the same route logic (including waitOn, data, action, etc.)

Something about this pattern breaks Router.go('productView', {productCode: productCode}) when called from a form. Maybe having a path with both / and a param?

If this doesn't ring any bells, I'll make a small reproduction package. It's just odd because whatever it is it breaks in iron_url.js.

chrisbutler commented 7 years ago

@evolross where did you get the idea that you could provide an array for a path?

ahh, interesting. i never looked under the hood at the iron url parser... looks like you can provide an array to pillarjs/path-to-regexp

i think one issue is that the first parameter in route() is the path property, but you're setting it again in object you're passing

when i tried to do this.route(['/', '/:productCode'], {...}) i get Router.route requires a path that is a string or regular expression.

based on https://github.com/iron-meteor/iron-router/blob/devel/lib/router.js#L116, arrays are not valid for the path property... i also found this open issue which discusses this functionality https://github.com/iron-meteor/iron-router/issues/727

evolross commented 7 years ago

It does work though to pass an array to path. For example I have this route that works great:

this.route('appGateway', {
    path: ['/hello', '/hola', '/welcome'],      
});

I could break those all out and use Router.go calls to direct them to where they should go but that's an easy one.

Above, I could break that out as well but that route is about 100 lines of code: it has complex subscriptions, sub-object data queries and loading, complex action, etc. In the spirit of DRY, I don't want to have an extra 100 lines of duplicate code in my router code if possible.

This pattern actually works nicely:

this.route('productView', {
    path: ['/', '/:productCode'],               
    waitOn: function() {
       if(!this.params.productCode)
           this.params.productCode = '123456789';

       return Meteor.subscribe('productByCode', this.params.productCode);           
    ...

You just lose functionality to Router.go('productView', {productCode: productCode}) and you have to use Router.go('/') or Router.go('/123456789') instead.

Just curious, does Flow Router handle this any better?

chrisbutler commented 7 years ago

@evolross what you actually have there (and i avoided mentioning this before because it was tangential to the real issue), i believe, is an anti-pattern for several reasons...

think about this for a minute... if you do Router.go('appGateway') in your example above, which path is the router supposed to use?

chrisbutler commented 7 years ago

here's how routing is supposed to work:


this.route('productView', {
    path: '/products/:productCode',             
    waitOn: function() {
       var code = this.params.productCode || '123456789';
       return Meteor.subscribe('productByCode', code);           
   }
})
evolross commented 7 years ago

So given your above route/template with path /products/:productCode that you can pass any product code to and given also we need the / path to display that same route/template with a pre-set default product (e.g. 123456789) but don't want to "redirect/update" the URL. I want it to show default product 123456789 but for the path to remain somedomain.com (i.e. /). So no calling Router.redirect or Router.go because I won't want the path to update to /products/123456789.

How is this accomplished without completely duplicating the route code?

chrisbutler commented 7 years ago

that would also be considered and anti-pattern, i would say... there are almost certainly better ways to implement whatever business logic is driving this, but you have more information about it than i, so i digress

i think creating a products controller would let you accomplish this with minimal duplication

chrisbutler commented 7 years ago

p.s. the anti-patterns are:

  1. showing different routes without changing the url
  2. adding lots of complexity to avoid duplicating what... 5 lines of code?
evolross commented 7 years ago
  1. It's actually the same route with two different paths. I'm basically wanting to send my naked domain to a "product view" page with a default product while keeping the URL the same.
  2. As I mentioned above, it's a route with about 100 lines of code and a lot of subscription complexity downstream from the product hence wanting to respect DRY. I just simplified my example.

Will look into Route Controllers. And I should probably be using Flow Router or React Router at this point anyway.

chrisbutler commented 7 years ago

the url thing is basically a semantics argument, i guess... do you have a page where you list all the products?

you can move those 100 lines to a controller and have multiple routes that inherit from it

also, rather than duplicate, you could always refactor your logic into functions (or hooks) that both routes could call

flow router has been abandoned and not updated in a year... react router is a good option but totally and radically different from the other two

chrisbutler commented 7 years ago

if you didn't realize before, you can also keep it as one route the way you have it, just by doing what i mentioned originally but removing /products (assuming this is the only route in the app, otherwise you may have conflicts or issues depending on the load order, etc)

this.route('productView', {
    path: '/:productCode',              
    waitOn: function() {
       var code = this.params.productCode || '123456789';
       return Meteor.subscribe('productByCode', code);           
   }
})