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

Only snapshot frame visits that have actions #827

Merged
merged 1 commit into from
Dec 23, 2022
Merged

Only snapshot frame visits that have actions #827

merged 1 commit into from
Dec 23, 2022

Conversation

kevinmcconnell
Copy link
Collaborator

Frames that use data-turbo-action to promote their navigations to page visits require cache snapshots to be taken prior to them loading, in order for those snapshots to be accurate.

However snapshots can be expensive to create for complex pages. In cases where a frame is not being promoted, we can avoid taking that snapshot.

This helps with some performance issues observed when using frames on very large pages.

/cc @afcapel

seanpdoyle
seanpdoyle approved these changes Dec 22, 2022
View reviewed changes
Copy link
Contributor

@seanpdoyle seanpdoyle left a comment

Choose a reason for hiding this comment

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

I'm 👍 for this change!

I wonder how we might add test coverage to guard against regressing with a future change.

Based on this change, I've updated #430 to use the same guard clause.

@seanpdoyle
Copy link
Contributor

To pass the test suite in CI, you might want to review and merge #822, then rebase.

@kevinmcconnell
Copy link
Collaborator Author

Thanks @seanpdoyle. I'll look at getting that CI fixed up with that PR. (Will also check out #430 as soon as I get a chance. Sorry, have been a bit behind on reviews and the like!)

@kevinmcconnell
Only snapshot frame visits that have actions
2fe01d3 
Frames that use `data-turbo-action` to promote their navigations to page
visits require cache snapshots to be taken prior to them loading, in
order for those snapshots to be accurate.

However snapshots can be expensive to create for complex pages. In cases
where a frame is *not* being promoted, we can avoid taking that
snapshot.

This helps with some performance issues observed when using frames on
very large pages.
@kevinmcconnell kevinmcconnell force-pushed the frames/only-snapshot-when-promoting branch from 5513edb to 2fe01d3 Compare December 23, 2022 13:35
@kevinmcconnell kevinmcconnell merged commit b8ecc46 into main Dec 23, 2022
@kevinmcconnell kevinmcconnell deleted the frames/only-snapshot-when-promoting branch December 23, 2022 14:34
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://togithub.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://togithub.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://togithub.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://togithub.com/hotwired/turbo/releases/tag/v7.2.5)

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

#### What's Changed

- Only snapshot frame visits that have actions by
[@&#8203;kevinmcconnell](https://togithub.com/kevinmcconnell) in
[https://github.com/hotwired/turbo/pull/827](https://togithub.com/hotwired/turbo/pull/827)
- Skip Snapshot Caching for redirect visits by
[@&#8203;seanpdoyle](https://togithub.com/seanpdoyle) in
[https://github.com/hotwired/turbo/pull/804](https://togithub.com/hotwired/turbo/pull/804)
- Fix: Promoting lazy-loaded Frames by
[@&#8203;seanpdoyle](https://togithub.com/seanpdoyle) in
[https://github.com/hotwired/turbo/pull/790](https://togithub.com/hotwired/turbo/pull/790)
- Merge in remaining header elements (eg: link icons, manifest, meta and
title) by [@&#8203;scuml](https://togithub.com/scuml) in
[https://github.com/hotwired/turbo/pull/752](https://togithub.com/hotwired/turbo/pull/752)
- Allow users to define an async custom render function by
[@&#8203;manuelpuyol](https://togithub.com/manuelpuyol) in
[https://github.com/hotwired/turbo/pull/782](https://togithub.com/hotwired/turbo/pull/782)
- Allow for listening to turbo events raised from within the Shadow DOM
by [@&#8203;yuki24](https://togithub.com/yuki24) in
[https://github.com/hotwired/turbo/pull/802](https://togithub.com/hotwired/turbo/pull/802)
- Ensure turbo no longer uses replaceChildren by
[@&#8203;MaxHatfull](https://togithub.com/MaxHatfull) in
[https://github.com/hotwired/turbo/pull/808](https://togithub.com/hotwired/turbo/pull/808)
- Add better support for shadow DOM by
[@&#8203;yuki24](https://togithub.com/yuki24) in
[https://github.com/hotwired/turbo/pull/758](https://togithub.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://togithub.com/seanpdoyle) in
[https://github.com/hotwired/turbo/pull/835](https://togithub.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://togithub.com/lit/lit/blob/HEAD/packages/lit/CHANGELOG.md#&#8203;261)

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

##### Patch Changes

- [#&#8203;3526](https://togithub.com/lit/lit/pull/3526)
[`65e56655`](https://togithub.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://togithub.com/lit/lit/pull/3561)
[`e5c254e9`](https://togithub.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://togithub.com/lit/lit/blob/HEAD/packages/lit/CHANGELOG.md#&#8203;260)

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

##### Minor Changes

- [#&#8203;3522](https://togithub.com/lit/lit/pull/3522)
[`72fcf0d7`](https://togithub.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://togithub.com/lit/lit/commit/72fcf0d70b4f4644e080e9c375a58cf8fc35e9e8)]:
-
[@&#8203;lit/reactive-element](https://togithub.com/lit/reactive-element)[@&#8203;1](https://togithub.com/1).6.0
    -   lit-html@2.6.0

</details>

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

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

[Compare
Source](https://togithub.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://togithub.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==-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanpdoyle seanpdoyle seanpdoyle approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kevinmcconnell @seanpdoyle