Pausable rendering (PR #28)

[kirillplatonov]

Extracted changes for pausable rendering from original PR #28.

Usage example:

addEventListener('turbo:before-render', function(event) {
  event.preventDefault()
  // prepare for rendering
  event.detail.resume()
})

TODO

• Add tests for pausable rendering
• Fix existing tests

[domchristie]

With #177 in mind, would it worth standardising this pattern, making it usable in other cases e.g. with an Interception object?

addEventListener('turbo:before-render', async function ({ preventDefault, detail: { interception } }) {
  preventDefault()
  await animateOut()
  interception.complete()
})

preventDefault could even be aliased to interception.start:

addEventListener('turbo:before-render', async function ({ detail: { interception } }) {
  interception.start()
  await animateOut()
  interception.complete()
})

…or could something like this work?:

addEventListener('turbo:before-render', async function ({ detail: { intercept } }) {
  intercept(() =>  await animateOut())
})

[kirillplatonov]

With #177 in mind, would it worth standardising this pattern, making it usable in other cases e.g. with an Interception object?

addEventListener('turbo:before-render', async function ({ preventDefault, detail: { interception } }) {
  preventDefault()
  await animateOut()
  interception.complete()
})

preventDefault could even be aliased to interception.start:

addEventListener('turbo:before-render', async function ({ detail: { interception } }) {
  interception.start()
  await animateOut()
  interception.complete()
})

…or could something like this work?:

addEventListener('turbo:before-render', async function ({ detail: { intercept } }) {
  intercept(() =>  await animateOut())
})

@domchristie Yeah. It makes sense to standardize this pattern and you the same approach for turbo:before-fetch-request. But instead of the Interceptor concept, I will try to do it using a pausable callback as well.

Talking about naming. I think event.preventDefault() is a pretty strong JS convention and it fits great for pausing render/request. To continue rendering/request it would be awesome to use something like event.detail.continue(). But unfortunately, continue is a reserved word in JS so I replaced it with event.detail.resume(). If you have a better name idea - LMK.

[dhh]

I feel the tension here in the sense that starting the interception and continuing it is happening in two different ways. If it was possible to do even.resume() then it would have felt much better. But event.preventDefault / event.detail.resume is an odd pairing. Also, it actually feels a bit odd to have event.preventDefault when the whole point is just to run some code before continuing on with the default.

Is it even necessary to manually do preventDefault? Couldn't the mere presence of this interception signify that? So that all you'd have to call is event.detail.resume(), after doing whatever you needed to do?

These aren't huge deals either way. Even what we have is OK. But worth exploring if we can refine it a bit.

[domchristie]

Is it even necessary to manually do preventDefault? Couldn't the mere presence of this interception signify that? So that all you'd have to call is event.detail.resume(), after doing whatever you needed to do?

Good point, I think the original code used the click handler code as a template. If we only need to check if a function has been called (e.g. interception.start()) then we might not need all the event handling stuff. With that in mind, I'm wondering if the following might work:

class Interception {
  constructor () {
    this.completed = new Promise((resolve) => this._resolve = resolve)
  }

  start () {
    this.started = true
  }

  complete (value) {
    this._resolve(value)
  }
}

---

// view.ts
async render(renderer: R) {
  if (this.renderer) {
    throw new Error("rendering is already in progress")
  }

  const { isPreview, shouldRender, newSnapshot: snapshot } = renderer
  if (shouldRender) {
    try {
      this.renderer = renderer
      this.prepareToRenderSnapshot(renderer)
      
      const interception = new Interception()
      this.delegate.viewWillRenderSnapshot(snapshot, isPreview, interception)
      if (interception.started) await interception.completed
      
      await this.renderSnapshot(renderer)
      this.delegate.viewRenderedSnapshot(snapshot, isPreview)
      this.finishRenderingSnapshot(renderer)
    } finally {
      delete this.renderer
    }
  } else {
    this.invalidate()
  }
}

I do take @kirillplatonov's point that preventDefault would make for a familiar API, although I've not really come across APIs that preventDefault then resume). I'm also mindful of how this might impact custom renderers, which has been mentioned previously. I'm not sure how this would look/work, so perhaps we can ignore this for now.

[domchristie]

An implementation of above: main...domchristie:pausable-rendering

[domchristie]

Using a single intercept function might be cleaner, e.g.:

addEventListener('turbo:before-render', async function ({ detail: { intercept } }) {
  intercept(() =>  await animateOut())
})

but it would require that the developer return a Promise, rather calling back (e.g. interception.complete()). A single function is perhaps more aesthetically pleasing and more magical, but a little less easy to understand?

[kirillplatonov]

An implementation of above: main...domchristie:pausable-rendering

It looks good to me. The implementation of the Interception class is really smart 👍 And we could easily reuse it for request interception as well. I do like the approach with separate start and complete calls a bit more. By just reading it you can easily understand what's going on.

Examples with this approach:

a) ES5

addEventListener('turbo:before-render', function(event) {
  event.detail.interception.start()
  // prepare for rendering
  event.detail.interception.complete()
})

b) ES6

addEventListener('turbo:before-render', function({ detail: { interception } }) {
  interception.start()
  // prepare for rendering
  interception.complete()
})

@dhh what do you think?

[domchristie]

Sorry to keep going with this! It does feel a bit redundant to have to explicitly call Interception#start, so here's how the might look with an intercept function: main...domchristie:pausable-rendering-intercept.

From the outside:

addEventListener('turbo:before-render', function({ detail: { intercept } }) {
  intercept(function (complete) {
    // prepare for rendering
    complete()
  })
})

[dhh]

So @domchristie, you'd just use this style when you wanted to pause, but a normal callback when you did not? The call structure is a little specific, but we can certainly document our way out of that. I like the idea that stop/start is handled automatically. Although couldn't we even assume that you want to complete after the function is run? Do we even need to do that explicitly? The main purpose here is for you to be able to stop render/request while you do some stuff, then once you're done, the render/request proceeds.

[domchristie]

So @domchristie, you'd just use this style when you wanted to pause, but a normal callback when you did not?

@dhh yes, for synchronous operations there'd be no need to use intercept e.g. if you wanted to query or modify newBody; but to cancel or pause rendering I think some kind of interception is needed.

I like the idea that stop/start is handled automatically. Although couldn't we even assume that you want to complete after the function is run?

Yes, I like this approach, although I think it might require that the intercept callback returns a Promise, whereas complete callback provides a little more flexibility.

I've updated main...domchristie:pausable-rendering-intercept with this approach here: 505c807.

[dhh]

Hmm. Yeah I guess I just have to give up on the hope that this could be as simple and elegant as a Ruby callback 😄. I think of the options we’ve reviewed so far, the one with the single complete function getting passed in is probably the best.

I’d vote we just go with that, and just make sure to document it clearly.

Thanks for exploring all these options! Nothing like seeing the code of all the alternatives to clear the path to a decision ✌️

[domchristie]

Yeah I guess I just have to give up on the hope that this could be as simple and elegant as a Ruby callback 😄

@dhh just to be sure I've understood, do you have an example of how this might look in the event handler?

[dhh]

@domchristie I was essentially hoping that we could make it such that the mere presence of a callback function could serve as the pointer to pause. I understand why that can't be so, though. We're picking between two different implementations that both have a fair chunk of incidental complexity. But also, it's not that big of a deal. This isn't something you're doing all the time. It needs to be possible, well-documented, but it doesn't need to be the most beautiful, succinct thing ever written.

So I say we just proceed with the version where complete() is passed in.

Sound good @kirillplatonov?

[dhh]

Nothing like sleeping on it. Contrast these:

// Option 1
addEventListener('turbo:before-render', function(event) {
  event.preventDefault()

  if (confirm('Continue rendering?')) {
    event.detail.resume()
  } else {
    alert('Rendering aborted')
  }
})


// Option 2a
addEventListener('turbo:before-render', function(event) {
  event.detail.intercept(function (complete) {
    if (confirm('Continue rendering?')) {
      complete()
    } else {
      alert('Rendering aborted')
    }
  })
}

// Option 2b
addEventListener('turbo:before-render', function({ detail: { intercept } }) {
  intercept(function (complete) {
    if (confirm('Continue rendering?')) {
      complete()
    } else {
      alert('Rendering aborted')
    }
  })
})

// Option 3
addEventListener('turbo:before-render', function(event) {
  event.detail.intercept(function () {
    return new Promise(function (resolve) {
      if (confirm('Continue rendering?')) {
        resolve()
      } else {
        alert('Rendering aborted')
      }
    })
  })
}

// Option 4
addEventListener('turbo:before-render', function(event) {
  event.detail.interception.start()

  if (confirm('Continue rendering?')) {
    event.detail.interception.complete()
  } else {
    alert('Rendering aborted')
  }
})

I think @kirillplatonov had the better option all along. Don't love the asymmetry, but I don't think we've managed to improve it with the alternatives. When you look at the a/b/c comparison here.

[domchristie]

@dhh Yeh, I think option 1 is nice because event.preventDefault is a familiar API, and resume is clear and reusable for other event interceptions. The downsides are that semantically, event.preventDefault does not describe the action well, and behind-the-scenes, the changeset is a little more complex because we have to check whether an event was cancelled. This logic is currently stored on the Session and the FrameController

By using an Interception, I think we can better organise this logic, for example:

• a FetchRequest has a before-fetch-request Interception
• a Renderer has a before-render Interception

As long as we can pass some kind of intercept function along to the event dispatcher, then it should be workable.

---

Another consideration when comparing these options: is there ever a case for aborting a render? Even if developers want to cancel the default render to perform their own (e.g. with DOM diffing), I'd imagine they'd still want the default to "resume" i.e. handle permanent elements, head scripts, scrolling etc.

If not, we don't need to test for this, and option 3 looks more inviting:

addEventListener('turbo:before-render', function({ detail: { intercept } }) {
  intercept(function () {
    if (confirm('Continue rendering?')) {
      return Promise.resolve()
    }
  })
}

(Related: #218 (comment) and #197 (comment))

---

Having said all this, I'm not 100% certain if Interception classes will definitively improve the complexities of the original preventDefault approach, and the case for making it a familiar API is a strong one.

[kirillplatonov]

I just pushed the refactored implementation of Option 1. It's much cleaner and simpler now.

If we all agreed that Option 1 is good enough for the API - let's move forward with it. Once merged I will add the same API for requests.

[dhh]

@domchristie Interesting idea re canceling outright. I say we wait for something to pop up to motivate that. But then we could have something like event.detail.resumeOnlySomeThings or whatever.

[domchristie]

@dhh I suppose my point was that aborting/cancelling a render probably won't ever be needed—developers will only want to customise how the body is replaced, then resume the default render process (handle permanent elements, head scripts, scrolling etc.); I believe this is why the rendering process was simplified to:

turbo/src/core/drive/page_renderer.ts

Lines 13 to 15 in cd27e50

	async render() {
	this.replaceBody()
	}

[dhh]

Yeah, I can definitely see that. Would be good to have a way to resume everything but rendering. Or resumeWithBody or something like that. But probably best to wait for a direct case to guide this!