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

Skip Snapshot Caching for redirect visits #804

Merged
merged 2 commits into from
Jan 4, 2023
Merged

Skip Snapshot Caching for redirect visits #804

merged 2 commits into from
Jan 4, 2023

Conversation

seanpdoyle
Copy link
Contributor

Closes hotwired/turbo#794

Reverts the implementation change made in hotwired/turbo#674, and in its place passes shouldCacheSnapshot: false alongside willRender: false for the action: "replace" Visit proposed when following a redirect.

In order to test this behavior, this commit introduces the readBodyMutationLogs test utility function, along with the window.bodyMutationLogs property and the BodyMutationLog type.

BodyMutationLog instances are pushed onto the log Array whenever the <body> element is replaced.

@seanpdoyle seanpdoyle mentioned this pull request Nov 25, 2022
Closed
@seanpdoyle
Copy link
Contributor Author

@kevinmcconnell depending on the response to #794 (comment), this is likely ready for code review if you're available.

@marcoroth marcoroth mentioned this pull request Dec 23, 2022
Closed
@seanpdoyle
Fix flaky Firefox tests
cea1621 
Some tests that visit the `navigation.html` fixture exercise scroll
behavior by visiting links and other content that is below the fold. The
`style="height: 200vh"` value declared on the page's `<section>` element
is arbitrary, but double the screen size to force content below the
fold.

Unluckily, it cuts off _right_ at the beginning of the new `<iframe>`
elements in Firefox browsers, which obscures the `<a>` element and
prevents link clicks.

To resolve that issue, this commit removes the `[style]` attribute,
since the content is long enough to be scrollable.
@seanpdoyle
Skip Snapshot Caching for redirect visits
44d28a1 
Closes [hotwired#794][]

Reverts the implementation change made in [hotwired#674][], and in
its place passes `shouldCacheSnapshot: false` alongside `willRender:
false` for the `action: "replace"` Visit proposed when following a
redirect.

In order to test this behavior, this commit introduces the
`readBodyMutationLogs` test utility function, along with the
`window.bodyMutationLogs` property and the `BodyMutationLog` type.

`BodyMutationLog` instances are pushed onto the log `Array` whenever the
`<body>` element is replaced.

[hotwired#794]: hotwired#794
[hotwired#674]: hotwired#674
@marcoroth marcoroth mentioned this pull request Jan 4, 2023
Closed
@dhh dhh merged commit e9692e7 into hotwired:main Jan 4, 2023
@seanpdoyle seanpdoyle deleted the issue-794 branch January 4, 2023 14:58
@seanpdoyle seanpdoyle mentioned this pull request Jan 17, 2023
Closed
@drjayvee
Copy link

Could I politely ask for a release of this fix?

I've now had several issues because of this. They're easy to fix using data-turbo-method="post", but finding instances of GET redirects is really hard, therefore users are running into bugs in our app because of #848 .

@jody90
Copy link

jody90 commented Jan 31, 2023

+1
I'm also looking forward to a new release which will include this fix.

@kevinmcconnell
Copy link
Collaborator

@drjayvee @jody90 v7.2.5 is being released now, which contains this fix.

@jody90
Copy link

jody90 commented Jan 31, 2023

Thank you! This was really fast.

michaelkedar referenced this pull request in google/osv.dev Feb 1, 2023
@renovate-bot
Update appengine-frontend (#978)
4c68935 
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [@hotwired/turbo](https://turbo.hotwired.dev)
([source](https://github.com/hotwired/turbo)) | [`7.2.4` ->
`7.2.5`](https://renovatebot.com/diffs/npm/@hotwired%2fturbo/7.2.4/7.2.5)
|
[![age](https://badges.renovateapi.com/packages/npm/@hotwired%2fturbo/7.2.5/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/npm/@hotwired%2fturbo/7.2.5/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/npm/@hotwired%2fturbo/7.2.5/compatibility-slim/7.2.4)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/npm/@hotwired%2fturbo/7.2.5/confidence-slim/7.2.4)](https://docs.renovatebot.com/merge-confidence/)
|
| [lit](https://lit.dev/) ([source](https://github.com/lit/lit)) |
[`2.5.0` -> `2.6.1`](https://renovatebot.com/diffs/npm/lit/2.5.0/2.6.1)
|
[![age](https://badges.renovateapi.com/packages/npm/lit/2.6.1/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/npm/lit/2.6.1/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/npm/lit/2.6.1/compatibility-slim/2.5.0)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/npm/lit/2.6.1/confidence-slim/2.5.0)](https://docs.renovatebot.com/merge-confidence/)
|
| [sass](https://github.com/sass/dart-sass) | [`1.57.1` ->
`1.58.0`](https://renovatebot.com/diffs/npm/sass/1.57.1/1.58.0) |
[![age](https://badges.renovateapi.com/packages/npm/sass/1.58.0/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/npm/sass/1.58.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/npm/sass/1.58.0/compatibility-slim/1.57.1)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/npm/sass/1.58.0/confidence-slim/1.57.1)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>hotwired/turbo</summary>

### [`v7.2.5`](https://github.com/hotwired/turbo/releases/tag/v7.2.5)

[Compare
Source](https://github.com/hotwired/turbo/compare/v7.2.4...v7.2.5)

#### What's Changed

- Only snapshot frame visits that have actions by
[@&#8203;kevinmcconnell](https://github.com/kevinmcconnell) in
[https://github.com/hotwired/turbo/pull/827](https://github.com/hotwired/turbo/pull/827)
- Skip Snapshot Caching for redirect visits by
[@&#8203;seanpdoyle](https://github.com/seanpdoyle) in
[https://github.com/hotwired/turbo/pull/804](https://github.com/hotwired/turbo/pull/804)
- Fix: Promoting lazy-loaded Frames by
[@&#8203;seanpdoyle](https://github.com/seanpdoyle) in
[https://github.com/hotwired/turbo/pull/790](https://github.com/hotwired/turbo/pull/790)
- Merge in remaining header elements (eg: link icons, manifest, meta and
title) by [@&#8203;scuml](https://github.com/scuml) in
[https://github.com/hotwired/turbo/pull/752](https://github.com/hotwired/turbo/pull/752)
- Allow users to define an async custom render function by
[@&#8203;manuelpuyol](https://github.com/manuelpuyol) in
[https://github.com/hotwired/turbo/pull/782](https://github.com/hotwired/turbo/pull/782)
- Allow for listening to turbo events raised from within the Shadow DOM
by [@&#8203;yuki24](https://github.com/yuki24) in
[https://github.com/hotwired/turbo/pull/802](https://github.com/hotwired/turbo/pull/802)
- Ensure turbo no longer uses replaceChildren by
[@&#8203;MaxHatfull](https://github.com/MaxHatfull) in
[https://github.com/hotwired/turbo/pull/808](https://github.com/hotwired/turbo/pull/808)
- Add better support for shadow DOM by
[@&#8203;yuki24](https://github.com/yuki24) in
[https://github.com/hotwired/turbo/pull/758](https://github.com/hotwired/turbo/pull/758)
- Fix: When applications embed [Google
Adsense](https://www.google.com/adsense/start/)-powered <iframe>
elements, the snippets they provide render them with a \[name] attribute
that's set to the empty string "" by
[@&#8203;seanpdoyle](https://github.com/seanpdoyle) in
[https://github.com/hotwired/turbo/pull/835](https://github.com/hotwired/turbo/pull/835)

**Full Changelog**:
hotwired/turbo@v7.2.4...v7.2.5

</details>

<details>
<summary>lit/lit</summary>

###
[`v2.6.1`](https://github.com/lit/lit/blob/HEAD/packages/lit/CHANGELOG.md#&#8203;261)

[Compare
Source](https://github.com/lit/lit/compare/lit@2.6.0...lit@2.6.1)

##### Patch Changes

- [#&#8203;3526](https://github.com/lit/lit/pull/3526)
[`65e56655`](https://github.com/lit/lit/commit/65e56655b73d22172647c1a748e7a907ad0227c0)
- Disable ShadyDOM noPatch in Node dev build. This fixes the issue of
throwing due to undefined `window`.

- [#&#8203;3561](https://github.com/lit/lit/pull/3561)
[`e5c254e9`](https://github.com/lit/lit/commit/e5c254e96cb5d0f770ec616332e231559325c5c5)
- Fix built-in shimming of `HTMLElement` for Node build of
`reactive-element` to respect existing `HTMLElement` in global

###
[`v2.6.0`](https://github.com/lit/lit/blob/HEAD/packages/lit/CHANGELOG.md#&#8203;260)

[Compare
Source](https://github.com/lit/lit/compare/lit@2.5.0...lit@2.6.0)

##### Minor Changes

- [#&#8203;3522](https://github.com/lit/lit/pull/3522)
[`72fcf0d7`](https://github.com/lit/lit/commit/72fcf0d70b4f4644e080e9c375a58cf8fc35e9e8)
- When running in Node, Lit now automatically includes minimal DOM shims
which are
sufficient for most SSR (Server Side Rendering) use-cases, removing the
need to
    import the global DOM shim from `@lit-labs/ssr`.

The new `@lit-labs/ssr-dom-shim` package has been introduced, which
exports an `HTMLElement`, `CustomElementRegistry`, and default
`customElements` singleton.

The existing `@lit-labs/ssr` global DOM shim can still be used, and is
compatible with the new package, because `@lit-labs/ssr` imports from
`@lit-labs/ssr-dom-shim`. Importing the global DOM shim adds more APIs
to the global object, such as a global `HTMLElement`, `TreeWalker`,
`fetch`, and other APIs. It is recommended that users try to remove
usage of the `@lit-labs/ssr` DOM shim, and instead rely on the more
minimal, automatic shimming that `@lit/reactive-element` now provides
automatically.

##### Patch Changes

- Updated dependencies
\[[`72fcf0d7`](https://github.com/lit/lit/commit/72fcf0d70b4f4644e080e9c375a58cf8fc35e9e8)]:
-
[@&#8203;lit/reactive-element](https://github.com/lit/reactive-element)[@&#8203;1](https://github.com/1).6.0
    -   lit-html@2.6.0

</details>

<details>
<summary>sass/dart-sass</summary>

###
[`v1.58.0`](https://github.com/sass/dart-sass/blob/HEAD/CHANGELOG.md#&#8203;1580)

[Compare
Source](https://github.com/sass/dart-sass/compare/1.57.1...1.58.0)

- Remove sourcemap comments from Sass sources. The generated sourcemap
comment
    for the compiled CSS output remains unaffected.

- Fix a bug in `@extend` logic where certain selectors with three or
more
combinators were incorrectly considered superselectors of similar
selectors
with fewer combinators, causing them to be incorrectly trimmed from the
    output.

- Produce a better error message for a number with a leading `+` or `-`,
a
    decimal point, but no digits.

- Produce a better error message for a nested property whose name starts
with
    `--`.

-   Fix a crash when a selector ends in an escaped backslash.

- Add the relative length units from CSS Values 4 and CSS Contain 3 as
known
    units to validate bad computation in `calc`.

##### Command Line Interface

- The `--watch` flag will now track loads through calls to
`meta.load-css()` as
    long as their URLs are literal strings without any interpolation.

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "before 6am on wednesday" in timezone
Australia/Sydney, Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config help](https://github.com/renovatebot/renovate/discussions) if
that's undesired.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/google/osv.dev).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC43My4zIiwidXBkYXRlZEluVmVyIjoiMzQuMTE0LjAifQ==-->
@blrB blrB mentioned this pull request Feb 5, 2023
Closed
@stevschmid stevschmid mentioned this pull request Jun 15, 2023
Open
marcelolx
marcelolx reviewed Dec 21, 2023
View reviewed changes
src/core/drive/visit.ts
Comment on lines +320 to +321
shouldCacheSnapshot: false,
willRender: false,
Copy link

Choose a reason for hiding this comment

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

@seanpdoyle I understand why we don't want to render again, but I wonder if the flow should be the opposite?

For example, now what happens is the following
-> User clicks a link
-> Link redirects to somewhere else
-> Fetch API follows the redirect
-> Turbo then renders the response
-> Turbo then follows the redirect (updating the history, for example)
-> Turbo then doesn't render the response (because it doesn't want to render it twice)

But shouldn't it be something like
-> User clicks a link
-> Link redirects to somewhere else
-> Fetch API follows the redirect
-> Turbo then SKIPS rendering the response (because it is a redirect)
-> Turbo then follows the redirect (updating the history, for example)
-> Turbo then RENDERS the response

Following this flow, it would be more akin to what browsers do, which is first visiting the redirected location and then rendering the content of the page of that location, wdyt?

NOTE: My assumption may be wrong, and I am just misunderstanding how things work, but I thought that it is worth bringing it up anyway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcelolx marcelolx marcelolx left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Body is replaced twice when following a redirect during Drive visit
6 participants
@seanpdoyle @drjayvee @jody90 @kevinmcconnell @marcelolx @dhh