Turbolinks extension, (optionally) include query string parameters in 'Turbolinks-Location' header?

[viktorvan]

I haven't used the ruby-on-rails implementation, so I don't know how this is implemented in that version. But the current Saturn implementation sets 'Turbolinks-Location' to ctx.Request.Path.Value, the virtual path. Which means any query string parameters are dropped when Turbolinks updates the location in the browser.

Maybe I am in the wrong here, but I would like to keep the query-string parameters. For example if a user wants to share a link with someone, I expect it to render the same view, given any query string parameters.

If you set the 'Turbolinks-Location' header to ctx.Request.Path.Value + ctx.Request.QueryString.Value it works as I expect it.

Maybe it can be an optional setting, or configurable in some way?

[Krzysztof-Cieslak]

TBF, I'm not an expert on Turbolinks - I've just ported initial implementation from RoR (and I'm not RoR expert so there is potential that my port is wrong/not-complete).

[viktorvan]

No problem, I can make a pull request with an overload use_turbolinks_with_config, or something along those lines. And then we can see how that turns out.

[viktorvan]

I finally had a chance to look closer at this, and I can think of some alternatives.

1. We make a breaking change to the current implementation and make it set the 'Turbolinks-Location' header to ctx.Request.Path.Value + ctx.Request.QueryString.Value. Personally this is what I would expect the location to be, but I could of course be wrong.

2. The second alternative is to keep the current behaviour, to not break anything, but make the location configurable, something along these lines (I haven't tested it yet):

// Lot's of current implementation omitted...

module TurbolinksHelpers =
  type LocationConfig = HttpContext -> StringValues

  let internal defaultLocation : LocationConfig = fun ctx -> StringValues ctx.Request.Path.Value
  let internal handleTurbolinks (ctx: HttpContext, locationConfig) =
    if isTurbolink ctx then
      let location =
        Option.map (fun c -> c ctx) locationConfig
        |> Option.defaultValue (defaultLocation ctx)
      ctx.Response.Headers.Add("Turbolinks-Location", location)

///HttpHandler enabling Turbolinks support for given pipelines
let turbolinks (nxt : HttpFunc) (ctx : HttpContext) locationConfig : HttpFuncResult =
  TurbolinksHelpers.handleTurbolinks (ctx, locationConfig)
  nxt ctx

type TurbolinksMiddleware (next: RequestDelegate, locationConfig) =
  member __.Invoke(ctx: HttpContext) =
    ctx.Response.OnStarting((fun ctx ->
      TurbolinksHelpers.handleTurbolinks (unbox<_> ctx, locationConfig)
      Task.CompletedTask
    ), ctx)
    next.Invoke(ctx)

type Saturn.Application.ApplicationBuilder with

  [<CustomOperation("use_turbolinks")>]
  ///Enable turbolinks supports for whole application (all endpoints)
  member __.UseTurbolinks(state) =
      let middleware (app : IApplicationBuilder) =
        app.UseMiddleware<TurbolinksMiddleware>(TurbolinksHelpers.defaultLocation)

      { state with AppConfigs = middleware::state.AppConfigs }

  [<CustomOperation("use_turbolinks_with_config")>]
  member __.UseTurbolinks(state, (config: TurbolinksHelpers.LocationConfig)) =
      let middleware (app : IApplicationBuilder) =
        app.UseMiddleware<TurbolinksMiddleware>(config)

      { state with AppConfigs = middleware::state.AppConfigs }

I am just concerned that this adds some unneeded complexity to configure Turbolinks, if option 1. is the one most people are after?

3. Another option is a combination of 1. and 2., where we change the default behaviour to be Path + QueryString and then also add use_turbolinks_with_config.

Any thoughts?

[Krzysztof-Cieslak]

I think we should go with option 1. - as far as I understand, correct behaviour is the result of my misunderstanding of how original RoR implementation works rather than some actual decision. So it's just a bug that we need to fix.