Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Upgrading to 7.1.rc3 doesn't pass parameters outside turboframe #465

Closed
acetinick opened this issue Nov 23, 2021 · 9 comments · Fixed by #466
Closed

Upgrading to 7.1.rc3 doesn't pass parameters outside turboframe #465

acetinick opened this issue Nov 23, 2021 · 9 comments · Fixed by #466

Comments

@acetinick
Copy link

Since upgrading from rc2 to rc3, it now strips out parameters from the pagy gem so it won't pass parameters to the server. It removes the page parameter after clicking on it, and the server doesn't recieve any parameters in the request.

FYI , If I wrap my table inside a it will work fine.

image
@seanpdoyle
Copy link
Contributor

I have a hunch at what's at the root of this, but I'm very worried this wasn't uncovered by failures in the test suite.

I'll open a PR to resolve the issue and add more thorough test coverage.

@acetinick
Copy link
Author

Yea looking at it more, it looks like its doing it for all standard links outside of turbo frames.

Looking at a few of the tests in the PR looks like most assume submit tags inside a form tag, in ths scenario its just an tag outside of any form/turbo-frame, and no data-turbo-method attribute.

@seanpdoyle
Copy link
Contributor

That's a good point, I'll expand that coverage to contain links.

What worries and surprises me is that the rest of the suit passed with broken behavior in such an integral feature.

@Intrepidd
Copy link
Contributor

I have a similar issue where I have a page with links to itself with query params and clicking on those links don't change the page content anymore with RC3

@tortuetorche
Copy link

tortuetorche commented Nov 23, 2021 •
edited
Loading

Hi folks,

I've also some issues when Turbo cache is opting out (<meta name="turbo-cache-control" content="no-cache">).
When I go to previous pages with back and forward buttons of my Web browser (Firefox or Chromium) a full page (from the latest Turbo Drive or full page navigation) is restored instead of the expected Turbo Frame.

Edit: I opened a new issue: #467

Have a good day,
Tortue Torche

seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Nov 23, 2021
@seanpdoyle
Restore searchParams to Visit and Frame Navigation
2c85b13 
Closes [hotwired#456][]

The changes made in [hotwired#464][] resolved an issue in how
`GET` requests merged query parameters into their `URL` instance.
Unfortunately, the changes were too loosely scoped, and they affected
_all_ `GET` requests, including those initiated by `<a>` elements with
`[href]` attributes that declare search parameters directly (that *do
not* need to be "merged" into anything).

The [change][] made in [hotwired#464][] to conditionally include
the `FetchRequest.body` based on the request's verb enables the changes
made in this commit.

In response to that change, this commit moves the
`mergeFormDataEntries()` function out of the `FetchMethod` module and
into the `FormSubmission` class. Since merging _from_ `FormData`
instances _into_ a `URL` is a concern of `GET`-powered Form Submissions,
moving the function out of the `FetchRequest` further simplifies that
class. Move the `mergeFormDataEntries()` to the `FormSubmission`
constructor to re-write its `location` property's [searchParams][] prior
to constructing the instance's `FetchRequest`.

In addition to that change, this commit removes the `FetchRequestBody`
union type that combined `URLSearchParams` and `FormData`. In its place,
change the `FetchRequest` to only accept `FormData` instances, since any
`URLSearchParams` management will occur outside that class.

[hotwired#456]: hotwired#465
[hotwired#464]: hotwired#464
[change]: hotwired@6906e57#diff-68b647dc2963716dc27c070f261d0b942ee9c00be7c4149ecb3a5acd94842d40R119
[searchParams]: https://developer.mozilla.org/en-US/docs/Web/API/URL/searchParams
seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Nov 23, 2021
@seanpdoyle
Restore searchParams to Visit and Frame Navigation
718a876 
Closes [hotwired#456][]

The changes made in [hotwired#464][] resolved an issue in how
`GET` requests merged query parameters into their `URL` instance.
Unfortunately, the changes were too loosely scoped, and they affected
_all_ `GET` requests, including those initiated by `<a>` elements with
`[href]` attributes that declare search parameters directly (that *do
not* need to be "merged" into anything).

The [change][] made in [hotwired#464][] to conditionally include
the `FetchRequest.body` based on the request's verb enables the changes
made in this commit.

In response to that change, this commit moves the
`mergeFormDataEntries()` function out of the `FetchMethod` module and
into the `FormSubmission` class. Since merging _from_ `FormData`
instances _into_ a `URL` is a concern of `GET`-powered Form Submissions,
moving the function out of the `FetchRequest` further simplifies that
class. Move the `mergeFormDataEntries()` to the `FormSubmission`
constructor to re-write its `location` property's [searchParams][] prior
to constructing the instance's `FetchRequest`.

In addition to that change, this commit removes the `FetchRequestBody`
union type that combined `URLSearchParams` and `FormData`. In its place,
change the `FetchRequest` to only accept `FormData` instances, since any
`URLSearchParams` management will occur outside that class.

[hotwired#456]: hotwired#465
[hotwired#464]: hotwired#464
[change]: hotwired@6906e57#diff-68b647dc2963716dc27c070f261d0b942ee9c00be7c4149ecb3a5acd94842d40R119
[searchParams]: https://developer.mozilla.org/en-US/docs/Web/API/URL/searchParams
@seanpdoyle seanpdoyle mentioned this issue Nov 23, 2021
Merged
@seanpdoyle
Copy link
Contributor

@acetinick @Intrepidd I've opened #466 to try and resolve the searchParams issue.

@tortuetorche what you're describing sounds unrelated to the original issue outlined here. Could you open a new Issue?

seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Nov 23, 2021
@seanpdoyle
Restore searchParams to Visit and Frame Navigation
5e68bd8 
Closes [hotwired#456][]

The changes made in [hotwired#464][] resolved an issue in how
`GET` requests merged query parameters into their `URL` instance.
Unfortunately, the changes were too loosely scoped, and they affected
_all_ `GET` requests, including those initiated by `<a>` elements with
`[href]` attributes that declare search parameters directly (that *do
not* need to be "merged" into anything).

The [change][] made in [hotwired#464][] to conditionally include
the `FetchRequest.body` based on the request's verb enables the changes
made in this commit.

In response to that change, this commit moves the
`mergeFormDataEntries()` function out of the `FetchMethod` module and
into the `FormSubmission` class. Since merging _from_ `FormData`
instances _into_ a `URL` is a concern of `GET`-powered Form Submissions,
moving the function out of the `FetchRequest` further simplifies that
class. Move the `mergeFormDataEntries()` to the `FormSubmission`
constructor to re-write its `location` property's [searchParams][] prior
to constructing the instance's `FetchRequest`.

[hotwired#456]: hotwired#465
[hotwired#464]: hotwired#464
[change]: hotwired@6906e57#diff-68b647dc2963716dc27c070f261d0b942ee9c00be7c4149ecb3a5acd94842d40R119
[searchParams]: https://developer.mozilla.org/en-US/docs/Web/API/URL/searchParams
@dhh dhh closed this as completed in #466 Nov 23, 2021
dhh pushed a commit that referenced this issue Nov 23, 2021
@seanpdoyle
Restore searchParams to Visit and Frame Navigation (#466)
b419c0d 
* Restore searchParams to Visit and Frame Navigation

Closes [#456][]

The changes made in [#464][] resolved an issue in how
`GET` requests merged query parameters into their `URL` instance.
Unfortunately, the changes were too loosely scoped, and they affected
_all_ `GET` requests, including those initiated by `<a>` elements with
`[href]` attributes that declare search parameters directly (that *do
not* need to be "merged" into anything).

The [change][] made in [#464][] to conditionally include
the `FetchRequest.body` based on the request's verb enables the changes
made in this commit.

In response to that change, this commit moves the
`mergeFormDataEntries()` function out of the `FetchMethod` module and
into the `FormSubmission` class. Since merging _from_ `FormData`
instances _into_ a `URL` is a concern of `GET`-powered Form Submissions,
moving the function out of the `FetchRequest` further simplifies that
class. Move the `mergeFormDataEntries()` to the `FormSubmission`
constructor to re-write its `location` property's [searchParams][] prior
to constructing the instance's `FetchRequest`.

[#456]: #465
[#464]: #464
[change]: 6906e57#diff-68b647dc2963716dc27c070f261d0b942ee9c00be7c4149ecb3a5acd94842d40R119
[searchParams]: https://developer.mozilla.org/en-US/docs/Web/API/URL/searchParams

* Attempt to fix flaky Scroll Restoration test

There is a common test failure in CI:

```
× firefox on linux 5.11.0-1020-azure - ScrollRestorationTests - landing on an anchor (0.02s)
444
    TypeError: Cannot read property 'slice' of null
445
      at RemoteChannel.read @ src/tests/helpers/remote_channel.ts:14:44
```

This commit addresses it in two ways. First, ensures that the
`scroll_restoration.html` page loads the
`src/tests/fixtures/test.js` script to wire-up the remote channels (like
`window.eventLogs` and `window.mutationLogs`).

Next, it modifies the test itself to await the `this.nextBody` hook, to
ensure that the page load completes before checking the scroll.
@dhh
Copy link
Member

dhh commented Nov 23, 2021

@acetinick @Intrepidd Lemme know when you've had a chance to test #466. Would like to move to a 7.1.0 final, if this is the last blocker!

@Intrepidd
Copy link
Contributor

@dhh works for me on main when it didn't on 7.1.rc3 :) Thanks @seanpdoyle

@acetinick
Copy link
Author

@dhh @seanpdoyle yep works great now :)

many thanks @seanpdoyle amazing work as always!

@maxnovee maxnovee mentioned this issue Nov 24, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Restore searchParams to Visit and Frame Navigation seanpdoyle/turbo
5 participants
@dhh @acetinick @Intrepidd @seanpdoyle @tortuetorche