`resume()` does not require `value` by dahlbyk · Pull Request #854 · hotwired/turbo

dahlbyk · 2023-01-27T09:29:53Z

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:

turbo/src/http/fetch_request.ts

Lines 172 to 182 in f04a21c

    
           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 }

marcoroth

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

resume() does not require value

d51fd96

marcoroth reviewed Jan 27, 2023

View reviewed changes

dhh merged commit 483ef32 into hotwired:main Feb 24, 2023

dahlbyk deleted the resume(value) branch February 24, 2023 14:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`resume()` does not require `value`#854

`resume()` does not require `value`#854
dhh merged 1 commit intohotwired:mainfrom
dahlbyk:resume(value)

dahlbyk commented Jan 27, 2023

Uh oh!

marcoroth left a comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

	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

	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

Conversation

dahlbyk commented Jan 27, 2023

Uh oh!

marcoroth left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants