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.

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

resume() does not require value #854

Merged
merged 1 commit into from
Feb 24, 2023
Merged

resume() does not require value #854

merged 1 commit into from
Feb 24, 2023

Conversation

dahlbyk
Copy link
Contributor

@dahlbyk dahlbyk commented Jan 27, 2023

Pausing Requests documents resume() usage without requiring a value, but TypeScript disagrees.

TS2554: Expected 1 arguments, but got 0.

Workaround is to call resume(undefined), but more representative types would be better. The signature matches the Promise callback's resolve parameter here, where you'll note the awaited result is discarded:

turbo/src/core/view.ts

Lines 92 to 95 in f04a21c

const renderInterception = new Promise((resolve) => (this.resolveInterceptionPromise = resolve))
const options = { resume: this.resolveInterceptionPromise, render: this.renderer.renderElement }
const immediateRender = this.delegate.allowsImmediateRender(snapshot, options)
if (!immediateRender) await renderInterception

A similar pattern is found here, where again the resolved value is discarded:

const requestInterception = new Promise((resolve) => (this.resolveRequestPromise = resolve))
const event = dispatch<TurboBeforeFetchRequestEvent>("turbo:before-fetch-request", {
cancelable: true,
detail: {
fetchOptions,
url: this.url,
resume: this.resolveRequestPromise,
},
target: this.target as EventTarget,
})
if (event.defaultPrevented) await requestInterception


The other option would be to remove the resume() parameter, but technically that's a breaking change for TypeScript users using resume(undefined). Easy enough to fix, of course. That could look something like:

@@ -7,3 +7,3 @@ import { getAnchor } from "./url"
 export interface ViewRenderOptions<E> {
-  resume: (value: any) => void
+  resume: () => void
   render: Render<E>
@@ -30,3 +30,3 @@ export abstract class View<
   private resolveRenderPromise = (_value: any) => {}
-  private resolveInterceptionPromise = (_value: any) => {}
+  private resolveInterceptionPromise = () => {}
 
@@ -91,3 +91,3 @@ export abstract class View<
 
-        const renderInterception = new Promise((resolve) => (this.resolveInterceptionPromise = resolve))
+        const renderInterception = new Promise((resolve) => (this.resolveInterceptionPromise = resolve as () => void))
         const options = { resume: this.resolveInterceptionPromise, render: this.renderer.renderElement }

Copy link
Member

@marcoroth marcoroth left a comment

Choose a reason for hiding this comment

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

I agree, marking the argument as optional is the more preferable solution here 👍🏼

@dhh dhh merged commit 483ef32 into hotwired:main Feb 24, 2023
@dahlbyk dahlbyk deleted the resume(value) branch February 24, 2023 14:31
another-rex referenced this pull request in google/osv.dev Mar 6, 2023
[![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.5` ->
`7.3.0`](https://renovatebot.com/diffs/npm/@hotwired%2fturbo/7.2.5/7.3.0)
|
[![age](https://badges.renovateapi.com/packages/npm/@hotwired%2fturbo/7.3.0/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/npm/@hotwired%2fturbo/7.3.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/npm/@hotwired%2fturbo/7.3.0/compatibility-slim/7.2.5)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/npm/@hotwired%2fturbo/7.3.0/confidence-slim/7.2.5)](https://docs.renovatebot.com/merge-confidence/)
|
| [sass](https://github.com/sass/dart-sass) | [`1.58.0` ->
`1.58.3`](https://renovatebot.com/diffs/npm/sass/1.58.0/1.58.3) |
[![age](https://badges.renovateapi.com/packages/npm/sass/1.58.3/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/npm/sass/1.58.3/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/npm/sass/1.58.3/compatibility-slim/1.58.0)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/npm/sass/1.58.3/confidence-slim/1.58.0)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

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

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

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

#### What's Changed

- Don't break out of frames when frame missing by
[@&#8203;kevinmcconnell](https://github.com/kevinmcconnell) in
[https://github.com/hotwired/turbo/pull/863](https://github.com/hotwired/turbo/pull/863)
- Respect `turbo-visit-control` for frame requests by
[@&#8203;kevinmcconnell](https://github.com/kevinmcconnell) in
[https://github.com/hotwired/turbo/pull/867](https://github.com/hotwired/turbo/pull/867)
- Allow changing the submitter text during form submission by
[@&#8203;afcapel](https://github.com/afcapel) in
[https://github.com/hotwired/turbo/pull/869](https://github.com/hotwired/turbo/pull/869)
- Form submissions from frames should clear cache by
[@&#8203;kevinmcconnell](https://github.com/kevinmcconnell) in
[https://github.com/hotwired/turbo/pull/882](https://github.com/hotwired/turbo/pull/882)
- Deprecate `[data-turbo-cache=false]` in favor of
`[data-turbo-temporary]` by
[@&#8203;packagethief](https://github.com/packagethief) in
[https://github.com/hotwired/turbo/pull/871](https://github.com/hotwired/turbo/pull/871)
- `resume()` does not require `value` by
[@&#8203;dahlbyk](https://github.com/dahlbyk) in
[https://github.com/hotwired/turbo/pull/854](https://github.com/hotwired/turbo/pull/854)
- Rename `isIdempotent` to `isSafe` by
[@&#8203;kevinmcconnell](https://github.com/kevinmcconnell) in
[https://github.com/hotwired/turbo/pull/883](https://github.com/hotwired/turbo/pull/883)

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

</details>

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

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

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

-   No user-visible changes.

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

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

##### Command Line Interface

-   Add a timestamp to messages printed in `--watch` mode.

- Print better `calc()`-based suggestions for `/`-as-division expression
that
    contain calculation-incompatible constructs like unary minus.

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

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

- Emit a unitless hue when serializing `hsl()` colors. The `deg` unit is
    incompatible with IE, and while that officially falls outside our
compatibility policy, it's better to lean towards greater compatibility.

</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:eyJjcmVhdGVkSW5WZXIiOiIzNC4xMjUuMSIsInVwZGF0ZWRJblZlciI6IjM0LjE0Ni4yIn0=-->
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.

3 participants