js-routes icon indicating copy to clipboard operation
js-routes copied to clipboard

Options style route params

Open mkdynamic opened this issue 1 year ago • 8 comments

This may be TypeScript specific, but with the native Rails route helpers, you can call in a couple ways. For example, assume Lead=1, Team=2 and App=3:

irb(main):002:0> app.lead_path({id: 1, team_id: 2, app_id: 3})
=> "/teams/2/apps/3/leads/1"
irb(main):003:0> app.lead_path(1, {team_id: 2, app_id: 3})
=> "/teams/2/apps/3/leads/1"
irb(main):004:0> app.lead_path(2, 3, 1)
=> "/teams/2/apps/3/leads/1"

When I generate the routes.js along with the types in the routes.d.ts, it only permits the last style:

export const leadPath: ((
  teamId: RequiredRouteParameter,
  appId: RequiredRouteParameter,
  id: RequiredRouteParameter,
  options?: {format?: OptionalRouteParameter} & RouteOptions
) => string) & RouteHelperExtras;

Not sure if this is TypeScript specific or not? Would it be feasible to support all three styles? The first one in particular is very useful where you have a bunch of shared options in nested resources and is more amenable to adding runtime default options via React Contexts, i.e. leadPath({ ...defaultParams, id: 1 }).

mkdynamic avatar Aug 05 '23 06:08 mkdynamic

The route does support this type of call at the Javascript level. However, it is not described in Typescript. Not so many people use this style and find it useful, but admit that in theory we need to describe all of the possible calls which is pretty hard to do and also makes definitions file super large. I am kind of sceptical about implementing that now. Let me think about it for a few days. Maybe the best thing is to introduce a generator option.

bogdan avatar Aug 06 '23 06:08 bogdan

@bogdan Ah gotcha, it is actually supported but just not present in the type definitions... An option to include all styles would be my preference, but I understand the concern about a super large def file. Although since it's development only, perhaps it's ok? Either way, a config on the generator would be great.

In my specific use case, if I had to just pick one style, the object style (passing all the parameters as an object) would be the best option for me. Specifically I have a lot of routes that are nested under 2 parent params (team ID and app ID) and so being able to use the spread operator to pass those in every route is very helpful.

mkdynamic avatar Aug 06 '23 07:08 mkdynamic

One follow-up:

To solve this at the controller level, I sub-class default_url_options and include the team and app ID. However, I don't think there is a comparable option with JS routes? But correct me if I missed that. Since those params are dynamic I cannot set them statically during generation.

mkdynamic avatar Aug 06 '23 07:08 mkdynamic

You can always configure JsRoutes at per page level in JavaScript:

import {configure, config} from 'routes'

configure({
  default_url_options: {team_id: ..., app_id: ...},
});
config(); // current config

bogdan avatar Aug 06 '23 07:08 bogdan

Ah, thanks, I missed that. Will see if I can use this to solve my issue.

mkdynamic avatar Aug 06 '23 08:08 mkdynamic

I have an idea to make all required arguments optional in the route definition:

export const leadPath: ((
  teamId?: RequiredRouteParameter,
  appId?: RequiredRouteParameter,
  id?: RequiredRouteParameter,
  options?: {format?: OptionalRouteParameter} & RouteOptions
) => string) & RouteHelperExtras;

This in fact is how it works in ruby: lead_path(*args, **options). There are some downsides as variable names will not match their meaning. However, I don't see any way of solving it. The exact definition of your lead route can be exteremely complex as it may allow calls with same signatures but different outcome like:

app.lead_path(1, {team_id: 2, app_id: 3})
app.lead_path(2, {id: 1, app_id: 3})

Maybe variable names should not be a thing when all of them are optional anyway.

bogdan avatar Aug 06 '23 08:08 bogdan

FWIW. I am monkey-patching locally to produce types like this:

interface LeadPath extends RouteHelperExtras {
  (
    teamId: RequiredRouteParameter,
    appId: RequiredRouteParameter,
    programId: RequiredRouteParameter,
    id?: RequiredRouteParameter,
    options?: {format?: OptionalRouteParameter} & RouteOptions
  ): string;
  (
    options: {
      team_id: RequiredRouteParameter,
      app_id: RequiredRouteParameter,
      id: RequiredRouteParameter
    } & {format?: OptionalRouteParameter} & Optional<RouteOptions>
  ): string;
}

export const leadPath: LeadPath;

Excuse the terrible hacky code :)

module MonkeyPatchJsRoutes
  def body(absolute)
    if @configuration.dts?
      override_definition_body(absolute)
    else
      super
    end
  end

  def override_definition_body(absolute)
    interface_name = helper_name(absolute).camelize(:upper)

    position_args = []
    required_parts.map { |p| position_args << "#{apply_case(p)}: RequiredRouteParameter" }
    position_args << "options?: #{optional_parts_type} & RouteOptions"

    options_args = []
    required_parts.map { |p| options_args << "#{p}: RequiredRouteParameter" }

    <<~TS.strip << "//"
      #{interface_name};

      interface #{interface_name} extends RouteHelperExtras {
        (
          #{position_args.join(",\n    ")}
        ): string;
        (
          options: {
            #{options_args.join(",\n      ")}
          } & #{optional_parts_type} & Optional<RouteOptions>
        ): string;
      }
    TS
  end
end

JsRoutes::Route.prepend(MonkeyPatchJsRoutes)

This definitely makes the type definitions larger/more-complex – so totally understand if you are not up for incorporating this change. Otherwise, LMK and I will prepare a PR when I get some time.

mkdynamic avatar Oct 14 '23 06:10 mkdynamic

I think I stick with idea to add a generator option to make all required route parameters optional. I am open for a PR.

bogdan avatar Dec 25 '23 09:12 bogdan