Skip to content

Convert search parameters to form inputs when using data-turbo-stream#641

Closed
LVMBDV wants to merge 1 commit intohotwired:mainfrom
LVMBDV:stream-link-form-population
Closed

Convert search parameters to form inputs when using data-turbo-stream#641
LVMBDV wants to merge 1 commit intohotwired:mainfrom
LVMBDV:stream-link-form-population

Conversation

@LVMBDV
Copy link
Copy Markdown

@LVMBDV LVMBDV commented Jul 20, 2022

Fixes a problem pointed out recently on #612 by @seanabrahams and @kevinmcconnell.

Links with data-turbo-stream are converted to forms, and forms with method="GET" don't submit the query string part of their action. This can be circumvented by iterating through any query string parameters and adding them to the generated form as hidden inputs.

Note: MDN refers to what we usually call "query string parameters" as "search parameters". This is a bit confusing, but it's the correct terminology I guess :)

Links with `data-turbo-stream` are converted to forms, and forms with
method="GET" don't submit the query string part of their action. This
can be circumvented by iterating through any query string parameters and
adding them to the generated form as hidden inputs.

Note: MDN refers to what we usually call "query string parameters" as
"search parameters". This is a bit confusing, but it's the correct
terminology I guess :)
form.setAttribute("action", action)
form.setAttribute("hidden", "")

const searchParameters = new URLSearchParams(new URL(action, window.location.toString()).search)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way the parameters are encoded depend on the from's [method] (as discovered in #461 and changed to be in compliance with the HTML Specification).

In short, GET submissions ignore the [action] query parameters and override them with the name-value pairs of its <input> elements, while POST submissions merge the two sets of name-value pairings.

If this method were to generate <input> elements, it should do it in a way that works consistently for both verbs. That might mean iterating through the action URL's search params, then deleting them afterward:

linkClickIntercepted(link: Element, location: string): 
  const action = new URL(action, document.baseURI)
  const form = document.createElement("form")
  form.setAttribute("data-turbo", "true")
  form.setAttribute("hidden", "")

  for (const [name, value] of action.searchParams) {
    const input = document.create("input")
    input.type = "hidden"
    input.name = name
    input.value = value
  
    form.append(input)
  }

  action.search = ""
  form.setAttribute("action", action.toString())

  // ...
}

@kevinmcconnell
Copy link
Copy Markdown
Collaborator

I've been looking at a different approach to this problem as well. Originally it seemed like submitting data-turbo-stream links as forms would be a straightforward & safe way to do this (especially as we already have data-turbo-method doing something similar), but seeing that issue with parameters come up it's making me doubt that approach.

I've got a branch going where I'm trying a different tack -- rather than submit these stream links as GET forms, the navigation code can simply add the stream MIME type to its usual request if the link has data-turbo-stream (in much the way way as the FormSubmission adds it according to its own logic). I think that will actually be a less invasive change, and less likely to introduce any unexpected behaviour, since the only change to the request will be that extra item in the Accept header.

I think it would be worth looking at that approach before we settle on the right fix. I'll try to get a PR open as soon as I can (but it will probably be a few days, sorry; busy week at work!)

@seanpdoyle does that sound reasonable to you?

@seanpdoyle
Copy link
Copy Markdown
Contributor

@kevinmcconnell I'm in favor of that change.

I'll try to get a PR open as soon as I can (but it will probably be a few days, sorry; busy week at work!)

I could draft something up today, if that's OK with you.

@LVMBDV could you create a test or two exercising the implementation change in this pull request? If you're able to add them here, I'll make sure to get them passing on the alternative PR.

@kevinmcconnell
Copy link
Copy Markdown
Collaborator

I could draft something up today, if that's OK with you.

Absolutely, that would be great 👍

FWIW, the approach I was looking at is to add an acceptsStreamResponse option to VisitOptions, which Visit can refer to in a prepareHeadersForRequest. That way, the Session can set that option appropriately when handling clicks. Seems to work pretty well, and is a less intrusive change. The part I hadn't tackled yet is that the FrameController will (I think) also want something similar, for the cases where it's issuing the request. That's where I ran out of time :)

Of course you may have an entirely different approach in mind; just wanted to mention where I'd reached, in case it saves you any time.

@LVMBDV
Copy link
Copy Markdown
Author

LVMBDV commented Jul 21, 2022

Hi, I needed this for a specific use case I stopped pursuing to allocate more time to releasing an MVP. If the other approaches don't work out, I can refine this PR i.e. add tests and documentation but let's shelve this one until that's decided.

@dhh
Copy link
Copy Markdown
Member

dhh commented Aug 1, 2022

Solved via #647 instead.

@dhh dhh closed this Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants