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

Use 'will render' false for action 'replace' after GET redirect #516

Merged
merged 7 commits into from
Jun 19, 2022
Merged

Use 'will render' false for action 'replace' after GET redirect #516

merged 7 commits into from
Jun 19, 2022

Conversation

blrB
Copy link
Contributor

@blrB blrB commented Jan 20, 2022 •
edited
Loading

Todo list:

  • fix issue
  • pass previous tests
  • add tests

Closes #515

@blrB blrB changed the title Use will render false for action 'replace' after GET redirect Draft: Use will render false for action 'replace' after GET redirect Jan 20, 2022
@blrB blrB changed the title Draft: Use will render false for action 'replace' after GET redirect Draft: Use 'will render' false for action 'replace' after GET redirect Jan 20, 2022
@blrB blrB mentioned this pull request Jan 20, 2022
Closed
@blrB blrB force-pushed the double_rendering_for_get_redirect branch from 2afffb9 to c891dcd Compare January 20, 2022 22:39
@blrB blrB force-pushed the double_rendering_for_get_redirect branch from c891dcd to ba5a8a3 Compare January 20, 2022 22:41
@blrB blrB changed the title Draft: Use 'will render' false for action 'replace' after GET redirect Use 'will render' false for action 'replace' after GET redirect Jan 20, 2022
@marphi
Copy link

marphi commented Jan 26, 2022 •
edited
Loading

In my project I've noticed the same problem with redirects. Looks like this PR fix it. @blrB thx a lot!

@blrB blrB mentioned this pull request Feb 12, 2022
Closed
@dfritsch dfritsch mentioned this pull request Feb 15, 2022
Closed
@feliperaul feliperaul mentioned this pull request Apr 3, 2022
Merged
@blrB blrB force-pushed the double_rendering_for_get_redirect branch from 13e0cde to 03b0f35 Compare April 30, 2022 21:43
@blrB
Copy link
Contributor Author

blrB commented Apr 30, 2022 •
edited
Loading

I fixed eslint errors in my code. All other errors in files not from my branch.

➜  turbo git:(double_rendering_for_get_redirect) yarn lint                                               
yarn run v1.22.17
$ eslint . --ext .ts

/Users/babkou/tmp/turbo/src/core/drive/page_renderer.ts
  15:48  error  Insert `,`  prettier/prettier
  21:43  error  Insert `,`  prettier/prettier

/Users/babkou/tmp/turbo/src/core/native/browser_adapter.ts
  11:14  error  Replace `[key:·string]:·any` with `·[key:·string]:·any·`  prettier/prettier
  57:23  error  Insert `,`                                                prettier/prettier
  58:12  error  Insert `,`                                                prettier/prettier

/Users/babkou/tmp/turbo/src/core/session.ts
  131:33  error  Insert `,`  prettier/prettier

✖ 6 problems (6 errors, 0 warnings)
  6 errors and 0 warnings potentially fixable with the `--fix` option.

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
➜  turbo git:(double_rendering_for_get_redirect

@blrB
Copy link
Contributor Author

blrB commented Apr 30, 2022

All eslint errors from other code was fixed too. (Beacuse GitHub CI can't be valide with any errors)

➜  turbo git:(double_rendering_for_get_redirect) yarn lint
yarn run v1.22.17
$ eslint . --ext .ts
✨  Done in 6.64s.
➜  turbo git:(double_rendering_for_get_redirect)

@dhh dhh merged commit 88bf29a into hotwired:main Jun 19, 2022
@timpwbaker
Copy link

Thanks! This fixed this issue for me too!

seanpdoyle added a commit to seanpdoyle/turbo that referenced this pull request Aug 9, 2022
@seanpdoyle
Bugfix: Redirects and [data-turbo-cache=false]
4722714 
When redirecting to a page that contains elements marked with
`[data-turbo-cache="false"]`, those elements are removed _before_ the
initial render, instead of _after_ the render and _before_ the page is
cached.

This behavior seems to have stemmed from [hotwired#516][], which
was shipped in response to [hotwired#515][].

As an alternative to the `willRender: false` option passed to
`this.adapter.visitProposedToLocation` in `Visit.followRedirect`, the
implementation can instead [rely on the presence of the
`turbo-frame[complete]`][comment] to guard against double fetching.

To guard against regressions, this commit adds coverage for the unwanted
behavior by redirecting from `navigation.html` to `cache_observer.html`,
and asserting the presence of a `[data-turbo-cache="false"]` element
that resembles and application's Flash messaging.

[hotwired#515]: hotwired#515
[hotwired#516]: hotwired#516
[comment]: hotwired#515 (comment)
[hotwired#487]: hotwired#487
@seanpdoyle seanpdoyle mentioned this pull request Aug 9, 2022
Merged
seanpdoyle added a commit to seanpdoyle/turbo that referenced this pull request Aug 9, 2022
@seanpdoyle
Bugfix: Redirects and [data-turbo-cache=false]
7988314 
When redirecting to a page that contains elements marked with
`[data-turbo-cache="false"]`, those elements are removed _before_ the
initial render, instead of _after_ the render and _before_ the page is
cached.

This behavior seems to have stemmed from [hotwired#516][], which
was shipped in response to [hotwired#515][].

As an alternative to the `willRender: false` option passed to
`this.adapter.visitProposedToLocation` in `Visit.followRedirect`, the
implementation can instead [rely on the presence of the
`turbo-frame[complete]`][comment] to guard against double fetching.

To guard against regressions, this commit adds coverage for the unwanted
behavior by redirecting from `navigation.html` to `cache_observer.html`,
and asserting the presence of a `[data-turbo-cache="false"]` element
that resembles and application's Flash messaging.

[hotwired#515]: hotwired#515
[hotwired#516]: hotwired#516
[comment]: hotwired#515 (comment)
[hotwired#487]: hotwired#487
dhh pushed a commit that referenced this pull request Aug 11, 2022
@seanpdoyle
Bugfix: Redirects and [data-turbo-cache=false] (#674)
256418f 
When redirecting to a page that contains elements marked with
`[data-turbo-cache="false"]`, those elements are removed _before_ the
initial render, instead of _after_ the render and _before_ the page is
cached.

This behavior seems to have stemmed from [#516][], which
was shipped in response to [#515][].

As an alternative to the `willRender: false` option passed to
`this.adapter.visitProposedToLocation` in `Visit.followRedirect`, the
implementation can instead [rely on the presence of the
`turbo-frame[complete]`][comment] to guard against double fetching.

To guard against regressions, this commit adds coverage for the unwanted
behavior by redirecting from `navigation.html` to `cache_observer.html`,
and asserting the presence of a `[data-turbo-cache="false"]` element
that resembles and application's Flash messaging.

[#515]: #515
[#516]: #516
[comment]: #515 (comment)
[#487]: #487
@blrB blrB mentioned this pull request Oct 26, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Double fetch requests for eager-loaded frames
4 participants
@blrB @marphi @timpwbaker @dhh