railsware / js-routes

Brings Rails named routes to javascript
http://railsware.github.io/js-routes/
MIT License
1.61k stars 151 forks source link

Prefer default_url_options over implictly filling with params when passing options hash #289

Closed ajclaasen closed 3 years ago

ajclaasen commented 3 years ago

Hello,

Thank you for all of your work, it's been great to work with and really helpful!

The issue

I have routes set up similar this in routes.rb:

scope '(:locale)', locale: Regexp.new(I18n.available_locales.join('|')) do
  resources :notes
end

We end up with (/:locale)/survey_fill_outs/:id(.:format) in % rails routes as an URI pattern.

I want to fill in the locale by default unless specified:

  config.default_url_options = { locale: "en" }

Then I want to query this resource as JSON in the frontend:

Routes.notesPath(123, { format: "json" }));

This raises the ParametersMissing: Route missing required keys: id exception.

If I explicitly pass the locale as part of the options, the request works just fine:

Routes.notesPath(123, { locale: "en", format: "json" });
>> /en/notes/123.json

Research

If you pass a second parameter, it turns out that the first one overwrites the :locale parameter:

Routes.notesPath(123, 456, { format: "json" });
>> /123/notes/456.json
Routes.notesPath(123, 456, { locale: "en", format: "json" }); // Doesn't do it if you pass the locale
>> /en/notes/123.json
Routes.notesPath(123); // It works if no options hash is passed
>> /en/notes/123

I did some digging in the source, and it seems that the issue arises from the partitions_parameters method in the javascript side of the gem. It overwrites the locale instead of filling in the missing parameter.

It seems that that happens in this part: https://github.com/railsware/js-routes/blob/877b89cb5c90841b606e7f8916ac63a220f433d7/lib/routes.js#L240-L247

Because the default_url_options are not used as a base to be overwritten by the options hash here: https://github.com/railsware/js-routes/blob/877b89cb5c90841b606e7f8916ac63a220f433d7/lib/routes.js#L208

Proposal

Using default_url_options in combination with passing an explicit options hash seems to be currently incompatible if there are required parameters defined in the default_url_options. The default_url_options are not used for filling empty required query parameters if an options hash exists, but are used if the options hash is not passed.

The easiest fix would be to use a copy of the default_url_options as the initial value of parts_options.

Thanks again for all your work, it's been lovely to work with! 🎈

bogdan commented 3 years ago

Looks like a legit bug, thanks for your research. I've added my review to #290 .

bogdan commented 3 years ago

Fixed by #290.