Skip to content
This repository has been archived by the owner on Aug 31, 2021. It is now read-only.

RFC: Pass 'event' instead of 'request' #197

Closed
wants to merge 1 commit into from

Conversation

samertm
Copy link
Contributor

@samertm samertm commented Oct 6, 2016

This PR updates request handlers to take event instead of request as their first argument, as discussed in #196. Request handlers need access to event because people that build on top of sw-toolbox may need access to event.client and event.waitUntil.

I didn't move event.respondWith inside of the event handlers because it seemed potentially error-prone and more verbose, because event.respondWith has to be called synchronously inside of the handler.

Looking at this, I'm not 100% sure it's worth breaking backwards compatibility.

Two alternatives:

  • Pass in the event object as the last arg: handler(request, values, options, event)
  • Add the event object to "options" (which overloads the use of options, which is currently only to override cache options):
handler(request, values, options) {
  // use options.event here

Thoughts?

@gauntface
Copy link

@samertm sorry that this has been sat without response for so long.

My gut says that we aren't likely to land this as it feels like a big enough change to break people and we'd rather encourage use of the sw-helpers for this kind of use case (sw-helpers is essentially a breakdown of sw-toolbox and sw-precache with the intention of allowing developers to build higher level abstractions on top of it)

@addyosmani
Copy link
Contributor

addyosmani commented Feb 28, 2017

I'm going to echo Matt's comments here and suggest that any backwards compat breaking changes such as this be considered in sw-helpers instead of sw-toolbox. That would greatly help us keep toolbox and precache relatively stable-train solutions for the next quarter while we continue to evolve the next versions of them in the helpers project: https://github.com/googlechrome/sw-helpers.

Thank you once again for the PR and we appreciate your suggested changes!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants