Dispatch visit events on the initiator link or form #695
Conversation
src/core/drive/visit.ts
Outdated
| @@ -50,7 +50,8 @@ export type VisitOptions = { | |||
| restorationIdentifier?: string | |||
| shouldCacheSnapshot: boolean | |||
| frame?: string | |||
| acceptsStreamResponse: boolean | |||
| acceptsStreamResponse: boolean, | |||
| initiator?: HTMLAnchorElement | HTMLFormElement | |||
There was a problem hiding this comment.
Could we declare this property without the ? optional modifier? VisitOption instances are typically passed around as Partial<VisitOption> types, so the Partial makes it optional.
There was a problem hiding this comment.
Without ? TypeScript moans because it's not present in defaultOptions:
Property 'initiator' is missing in type '{ action: "advance"; historyChanged: false; visitCachedSnapshot: () => void; willRender: true; updateHistory: true; shouldCacheSnapshot: true; acceptsStreamResponse: false; }' but required in type 'VisitOptions'.
There was a problem hiding this comment.
Could its default value be document.documentElement, since that's what it's currently dispatched from?
There was a problem hiding this comment.
I'm not strongly opposed to this, but just wonder if having an undefined initiator usefully signals that the Visit is a programmatic one?
There was a problem hiding this comment.
I'm not strongly opposed to this, but just wonder if having an undefined initiator usefully signals that the Visit is a programmatic one?
I suppose it doesn't really matter since a programmatic visit could be determined if visit.initiator === document.documentElement
There was a problem hiding this comment.
Or if we want a default, perhaps initiator is the wrong name for this? i.e. for programmatic visits the HTML element doesn't "initiate" the visit, the event just gets dispatched on it. It's not as clear, but perhaps Visit#element would work?
There was a problem hiding this comment.
Default initiator implemented in: 36dd97b
src/core/session.ts
Outdated
| @@ -191,17 +191,17 @@ export class Session | |||
| ) | |||
| } | |||
|
|
|||
| followedLinkToLocation(link: Element, location: URL) { | |||
| followedLinkToLocation(link: HTMLAnchorElement, location: URL) { | |||
There was a problem hiding this comment.
While I appreciate the type narrowing, so long as the initiator can invoke dispatchEvent, we might be able to achieve the outcome without disrupting these interfaces.
I've been tempted to narrow this delegate method before, but I have a hunch that there is some Custom Element support reason that it's still Element.
There was a problem hiding this comment.
What would your advice be when updating the types here?:
https://github.com/hotwired/turbo/pull/695/files#diff-68b647dc2963716dc27c070f261d0b942ee9c00be7c4149ecb3a5acd94842d40R65
There was a problem hiding this comment.
95590c4 uses Element for all references
…723) The inclusion of the `initiator` in `VisitOptions` causes a JS crash in the Turbo iOS adapter. This is because Turbo iOS's implementation of `visitProposedToLocation` relies on being able to pass the entire options struct to native code. Initiator is an `Element`, which can't be passed in this way, resulting in a `DataCloneError` exception. There is some discussion on this [GitHub issue][]. We're reverting the change for the moment, until we have an implementation of it that works with all adapters. [GitHub issue]: #720 This reverts commit 8871817.
In a similar vein to #367, this pull request dispatches
turbo:visit,turbo:before-visit,turbo:before-fetch-request, andturbo:before-fetch-responseon the initiator link or form (where applicable), rather than solely on thehtmlelement.Why? When developing libraries that build on top of Turbo, it'd be nice to customise behaviour based on HTML attributes of the clicked link or submitted form. For example, if I were building an animation library and wished to disable a transition when clicking a particular element, I could do:
I could then use
event.target.dataset.animatein theturbo:visitevent to conditionally animate.Other examples are mentioned in #99.
Closes #99