New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optional parameter before required breaks route generation #231

Closed
gyfis opened this Issue Nov 22, 2017 · 14 comments

Comments

Projects
None yet
2 participants
@gyfis

gyfis commented Nov 22, 2017

We have an optional parameter in our path before a required one - (:locale) before :slug. When we include both the required parameter and the option hash in the path function while leaving out the first optional parameter, it fails with ParameterMissing. This doesn't happen for paths where there is no optional parameter before a required one.

Example path (generated by JsRoutes.generate!):

...
// blog_post => (/:locale)/blog/:slug(.:format)
// function(slug, options)
blog_post_path: Utils.route([["locale",false],["slug",true],["format",false]], {}, [2,[1,[2,[7,"/",false],[3,"locale",false]],false],[2,[7,"/",false],[2,[6,"blog",false],[2,[7,"/",false],[2,[3,"slug",false],[1,[2,[8,".",false],[3,"format",false]],false]]]]]]),
...

This works:

> Routes.blog_post_path("my-awesome-slug")
"/blog/my-awesome-slug"

This fails while it should've skipped the optional parameter (the same way as the above, only with including the option).

> Routes.blog_post_path("my-awesome-slug", {foo: 'bar'})
ParameterMissing {message: "Route parameter missing: slug"}

However, adding the optional param (locale) to the options object seems to work again.

> Routes.blog_post_path("my-awesome-slug", {foo: 'bar', locale: 'es'})
"/es/blog/my-awesome-slug"
@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan Nov 23, 2017

Collaborator

I tried this route in rails and it also raises an error:

ActionController::UrlGenerationError: No route matches {:action=>"foo", :controller=>"foo", :foo=>"bar", :locale=>"my-awesome-slug"}, missing required keys: [:slug]

Rails v5.1.3.

Can you ensure there is an incompatibility in behavior between Rails and JsRoutes?

Collaborator

bogdan commented Nov 23, 2017

I tried this route in rails and it also raises an error:

ActionController::UrlGenerationError: No route matches {:action=>"foo", :controller=>"foo", :foo=>"bar", :locale=>"my-awesome-slug"}, missing required keys: [:slug]

Rails v5.1.3.

Can you ensure there is an incompatibility in behavior between Rails and JsRoutes?

@gyfis

This comment has been minimized.

Show comment
Hide comment
@gyfis

gyfis Nov 23, 2017

I understand that Rails require the use of keyword arguments, which is not the same as the functions generated by JsRoutes.

We also tried to fill in all the arguments as js object (keyword arguments), and that did not work for some use-cases. Below is an example.

This works correctly for all kinds of optional arguments:

// blog_post => (/:locale)/blog/:slug(.:format)
...
Routes.blog_post_path({slug: 'my-awesome-blog', format: 'json'})
"/blog/my-awesome-blog.json"
...
Routes.blog_post_path({slug: 'my-awesome-blog'})
"/blog/my-awesome-blog"
...
Routes.blog_post_path({slug: 'my-awesome-blog', locale: 'es', format: 'json', x: 3})
"/es/blog/my-awesome-blog.json?x=3"

However, this does not work:

// user_page => /user/:id(.:format)
...
Routes.user_page_path({id: 3})
"/user/3"
...
Routes.user_page_path({id: 3, format: 'json', x: 3})
"/user/3"    <------ !!!

It seems that when there is no optional param before required param, the generated route does not meet the expectation. This is an assumption, and possibly a different issue.

gyfis commented Nov 23, 2017

I understand that Rails require the use of keyword arguments, which is not the same as the functions generated by JsRoutes.

We also tried to fill in all the arguments as js object (keyword arguments), and that did not work for some use-cases. Below is an example.

This works correctly for all kinds of optional arguments:

// blog_post => (/:locale)/blog/:slug(.:format)
...
Routes.blog_post_path({slug: 'my-awesome-blog', format: 'json'})
"/blog/my-awesome-blog.json"
...
Routes.blog_post_path({slug: 'my-awesome-blog'})
"/blog/my-awesome-blog"
...
Routes.blog_post_path({slug: 'my-awesome-blog', locale: 'es', format: 'json', x: 3})
"/es/blog/my-awesome-blog.json?x=3"

However, this does not work:

// user_page => /user/:id(.:format)
...
Routes.user_page_path({id: 3})
"/user/3"
...
Routes.user_page_path({id: 3, format: 'json', x: 3})
"/user/3"    <------ !!!

It seems that when there is no optional param before required param, the generated route does not meet the expectation. This is an assumption, and possibly a different issue.

@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan Nov 23, 2017

Collaborator

It looks like a true bug.... I will think about the fix.

Collaborator

bogdan commented Nov 23, 2017

It looks like a true bug.... I will think about the fix.

@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan Nov 23, 2017

Collaborator

The problem with that route is described here: https://github.com/railsware/js-routes#object-and-hash-distinction-issue

In your case, to get the correct route pass _options special key:

Routes.user_page_path({id: 3, format: 'json', x: 3, _options: true})
Collaborator

bogdan commented Nov 23, 2017

The problem with that route is described here: https://github.com/railsware/js-routes#object-and-hash-distinction-issue

In your case, to get the correct route pass _options special key:

Routes.user_page_path({id: 3, format: 'json', x: 3, _options: true})
@gyfis

This comment has been minimized.

Show comment
Hide comment
@gyfis

gyfis Nov 23, 2017

Thanks for the swift reply. If I understand it correctly, we should replace all our calls to Routes with _options: true and use that in the future to be solid? Do you think there might be another way or approach on how to solve these issues?

gyfis commented Nov 23, 2017

Thanks for the swift reply. If I understand it correctly, we should replace all our calls to Routes with _options: true and use that in the future to be solid? Do you think there might be another way or approach on how to solve these issues?

@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan Nov 23, 2017

Collaborator

We were thinking about it for a long time and didn't find any solution: JS treats hashes and objects in the same way.

On your end, I would rather use user_page_path(3, format: 'json', x: 3). I don't know what was the reason to push required parameters as options in the first place.

Collaborator

bogdan commented Nov 23, 2017

We were thinking about it for a long time and didn't find any solution: JS treats hashes and objects in the same way.

On your end, I would rather use user_page_path(3, format: 'json', x: 3). I don't know what was the reason to push required parameters as options in the first place.

@bogdan bogdan closed this Nov 23, 2017

@gyfis

This comment has been minimized.

Show comment
Hide comment
@gyfis

gyfis Nov 24, 2017

Using user_page_path(3, format: 'json', x: 3) raises Uncaught SyntaxError: missing ) after argument list. Writing it as user_page_path(3, {format: 'json', x: 3}) is getting back to the original issue described in this thread. I'm not really sure if there is another option to use that works instead of writing everything to the options object with the _options: true.

gyfis commented Nov 24, 2017

Using user_page_path(3, format: 'json', x: 3) raises Uncaught SyntaxError: missing ) after argument list. Writing it as user_page_path(3, {format: 'json', x: 3}) is getting back to the original issue described in this thread. I'm not really sure if there is another option to use that works instead of writing everything to the options object with the _options: true.

@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan Nov 24, 2017

Collaborator

Writing it as user_page_path(3, {format: 'json', x: 3}) is getting back to the original issue described in this thread

I can not reproduce that. Which version of rails do you use? What is the output of that call in the rails console?

Collaborator

bogdan commented Nov 24, 2017

Writing it as user_page_path(3, {format: 'json', x: 3}) is getting back to the original issue described in this thread

I can not reproduce that. Which version of rails do you use? What is the output of that call in the rails console?

@bogdan bogdan reopened this Nov 24, 2017

@gyfis

This comment has been minimized.

Show comment
Hide comment
@gyfis

gyfis Nov 24, 2017

This happens in JsRoutes, not Rails. Sorry for the confusion.

gyfis commented Nov 24, 2017

This happens in JsRoutes, not Rails. Sorry for the confusion.

@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan Nov 24, 2017

Collaborator

That doesn't change my question. In order to confirm the bug you need to give me a call that gives one thing in rails and other thing in JsRoutes. And that should not be the one caused by Hash <-> Object indistinction issue of JS

Collaborator

bogdan commented Nov 24, 2017

That doesn't change my question. In order to confirm the bug you need to give me a call that gives one thing in rails and other thing in JsRoutes. And that should not be the one caused by Hash <-> Object indistinction issue of JS

@gyfis

This comment has been minimized.

Show comment
Hide comment
@gyfis

gyfis Nov 24, 2017

I'll try to summarize our issues and solution.

For this path and params, JsRoutes and Rails behave differently.

get '/(:locale)/blog/:slug', to: 'blog#show', as: :blog_post
// blog_post => (/:locale)/blog/:slug(.:format)

Rails

> app.blog_post_path("slug")
ActionController::UrlGenerationError: No route matches {:action=>"show", :controller=>"blog", :locale=>"slug", :slug=>nil} missing required keys: [:slug]

JsRoutes

> Routes.blog_post_path("slug")
"/blog/slug"

However, with adding options JsRoutes raises error as well. Because we want to use other options, we need to move slug to options and enable _options: true.

That means that for paths with /(:locale), we need to move all keys to options object, and for paths without /(:locale), we can either pass the parameters directly (e.g. blog_post_path("slug")) or use the options object.

Because we want to be consistent in our usage of JsRoutes, we decided to use the options object path with _options: true. I think most of our confusion was created by the inconsistency of the parameter handling (described in the initial comment here).

Thanks for all the help, feel free to close the issue if you feel like that is not worth fixing.

gyfis commented Nov 24, 2017

I'll try to summarize our issues and solution.

For this path and params, JsRoutes and Rails behave differently.

get '/(:locale)/blog/:slug', to: 'blog#show', as: :blog_post
// blog_post => (/:locale)/blog/:slug(.:format)

Rails

> app.blog_post_path("slug")
ActionController::UrlGenerationError: No route matches {:action=>"show", :controller=>"blog", :locale=>"slug", :slug=>nil} missing required keys: [:slug]

JsRoutes

> Routes.blog_post_path("slug")
"/blog/slug"

However, with adding options JsRoutes raises error as well. Because we want to use other options, we need to move slug to options and enable _options: true.

That means that for paths with /(:locale), we need to move all keys to options object, and for paths without /(:locale), we can either pass the parameters directly (e.g. blog_post_path("slug")) or use the options object.

Because we want to be consistent in our usage of JsRoutes, we decided to use the options object path with _options: true. I think most of our confusion was created by the inconsistency of the parameter handling (described in the initial comment here).

Thanks for all the help, feel free to close the issue if you feel like that is not worth fixing.

@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan Nov 24, 2017

Collaborator

I can not reproduce that. Which version of Rails do you use?

I didn't understand that part after "However". Can you describe it in the same way as you described the part before "However".

Collaborator

bogdan commented Nov 24, 2017

I can not reproduce that. Which version of Rails do you use?

I didn't understand that part after "However". Can you describe it in the same way as you described the part before "However".

@gyfis

This comment has been minimized.

Show comment
Hide comment
@gyfis

gyfis Nov 24, 2017

We use Rails 4.2.7.1.

What I meant by the "However" part is

> Routes.blog_post_path("slug", {x: 3})

raises error in the same way Rails do.

gyfis commented Nov 24, 2017

We use Rails 4.2.7.1.

What I meant by the "However" part is

> Routes.blog_post_path("slug", {x: 3})

raises error in the same way Rails do.

@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan Nov 24, 2017

Collaborator

I still can not reproduce that bug. Are you sure you are using latest js-routes?

Regarding "However" part: this is by design - JsRoutes should behave the same way Rails does even when Rails is doing something weird. We can not invent our own spec how routes helpers should work.

Collaborator

bogdan commented Nov 24, 2017

I still can not reproduce that bug. Are you sure you are using latest js-routes?

Regarding "However" part: this is by design - JsRoutes should behave the same way Rails does even when Rails is doing something weird. We can not invent our own spec how routes helpers should work.

@bogdan bogdan closed this Jan 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment