WICG / navigation-api

The new navigation API provides a new interface for navigations and session history, with a focus on single-page application navigations.
https://html.spec.whatwg.org/multipage/nav-history-apis.html#navigation-api
484 stars 30 forks source link

Signal abort via an AbortController rather than directly on the signal #265

Closed shaseley closed 1 year ago

shaseley commented 1 year ago

Currently, NavigateEvent's signal is aborted directly via "signal abort", but we'd like to stop exporting "signal abort", having specs abort via a controller (like in userland). The tricky bit here is how to handle userland-created NavigateEvents. We could:

  1. Leave NavigateEventInit alone, make NavigateEvent have an internal AbortController, set NavigateEventInit's signal to the controller's signal. This doesn't change the API at all, but it's a little gross (need to store both a controller and signal, common case signal == controller.signal).

  2. Remove signal from NavigateEventInit, makeNavigateEvent have an internal AbortController, have NavigateEvent's signal property return the controller's signal. This is the cleanest IMO, but IIUC it could break some unit test functionality?

  3. NavigateEventInit takes an AbortController and NavigateEvents signal attribute return the controller's signal (credit: @natechapin). This is probably cleaner than (1), but changes the API shape and is a bit unusual (passing a controller).

@domenic thoughts on what you'd prefer here?

domenic commented 1 year ago

I think I lean toward a variant of (1). Which is, we don't necessarily have to associate every NavigateEvent with an AbortController: we only need to do that for spec-created ones. Because the spec only ever wants to signal-abort on spec-created NavigateEvents.

Another way of thinking about this, is that we change the spec code from having a handle to a NavigateEvent, to having a handle to a NavigateEvent + AbortController. We could even, in the spec, store it alongside the NavigateEvent; see https://whatpr.org/html/8502/nav-history-apis.html#ongoing-navigation-tracking .

That way, the interface of NavigateEvent itself remains: it just gets a signal. It doesn't know whether that signal came from other parts of the spec that create AbortControllers, or from userland AbortControllers. It remains like other events, a dumb property-bag.

shaseley commented 1 year ago

Okay yeah, makes sense; storing an AbortController alongside the NavigateEvent is pretty clean and makes sense conceptually (vs. storing it on the event).

domenic commented 1 year ago

Fixed by https://github.com/whatwg/html/pull/8502/commits/40c167d7ee9e4a68fab47037833030870607a554.