Navigations failing in Turbo iOS apps, due to initiator visit option

[kevinmcconnell]

In #695, an initiator: Element property is added to VisitOptions, in order to track the element that initiates the visit. This works great in the browser 👍

However it causes a problem for native apps that use the Turbo iOS adapter. The adapter's implementation of visitProposedToLocation passes the options through to native code using postMessage, and the presence of the Element causes it to fail with a DataCloneError exception. (Presumably because the Element type isn't supported by the underlying structuredClone operation.)

The result is that tapping links in Turbo iOS apps silently fails (aside from the console error that you can see in development).

This could be fixed in Turbo iOS, but In order to not break backwards compatibility with existing apps, it would be better to take the initiator property back out of the options.

One option could be to extend visitProposedToLocation with an additional argument (EventOptions perhaps?) and move the initiator property into it. That extra argument would be ignored by the existing Turbo iOS code, so it would be a backwards-compatible way to evolve that part of the adapter interface. Alternatively, we could try to sidestep visitProposedToLocation entirely with this, and set the initiator property on the Visit after creation.

What do you think @domchristie @seanpdoyle ?

[domchristie]

@kevinmcconnell thanks for testing this.

How about sanitising the options before calling adapter#visitProposedToLocation? Something like:

function sanitizeOptions (options) {
  for (const key in options) {
    try { structuredClone(options[key]) }
    catch (e) { delete options[key] }
  }
  return options
}

Ideally this would be done on the Turbo iOS side before calling postMessage, but as you mention: backwards compatibility :/

We should also make a note that VisitOptions need be transferable objects.

---

Another thought: it does seem a bit of a shame that native clients won't be able to make use of this feature. Perhaps initiator could be converted to an outerHTML string for the native case?

[kevinmcconnell]

Hi @domchristie

How about sanitising the options before calling adapter#visitProposedToLocation?

It's a good idea, but (unless I'm overlooking something here) the problem is that visitProposedToLocation is the route used by all adapters to initiate a visit (including the browser adapter). So sanitising the options in this way would stop the feature working.

If you only sanitise it for a specific adapter, that could stop the crash, but it also means that the feature won't be working for clients using that adapter. Browser clients will then have events fired on the initiating element, but in mobile clients those events will continue to fire on the document element as before. I think that would lead to confusion for anyone adding listeners to those events.

Ideally this would be done on the Turbo iOS side before calling postMessage, but as you mention: backwards compatibility :/

Yeah, agreed. I'll have a look at the iOS side to see if there's something useful we can do for future versions there. It wouldn't help with backwards compatibility, but maybe we can make things easier on ourselves in the future somehow.

One other idea could be to narrow the scope of this a bit so that it only affects the events that fire before the visitProposedToLocation step. I haven't tried this (so take it with a grain of salt :)) but it looks like you could include the initiating element in turbo:before-visit that way without affecting what's passed to the adapters. Maybe that's a useful compromise?

Either way.... until we have a version of this that works in all the adapters, I think we should consider reverting it in the meantime. It'll be great to have when it's ready, but currently it's a blocker for any iOS apps to use this version of Turbo. Does that seem reasonable?

[domchristie]

Hey @kevinmcconnell

So sanitising the options in this way would stop the feature working.

Ah of course! (I guess I was hoping that the event would be dispatched before, but it makes total sense it does not!)

Having mulled this over, I still think it makes sense for initiator to be part of VisitOptions but that we store and remove it before calling adapter.visitProposedToLocation, and reinstate it when creating the visit. I think that might be the only way to ensure this works with any adapter. The tricky part is, where does it make sense to store it? and how do we make sure that the visit is created with the correct initiator?

Here's a diff of what it might look like if we store the initiator on the Navigator: main...domchristie:turbo:initiator_with_native_support It's not ideal because we can't remove it in the stop method because that gets called prior to creating the visit. We could re-set it, in Navigator#startVisit, but that just feels dirty.

---

I'm thinking we could also do some clever stuff with TypeScript to ensure that visit options passed to an adapter are always Transferable?

[kevinmcconnell]

Yes, I think that sounds like it could work! Keeping this outside of the adapter seems like the key to making this work in a backwards compatible way.

If it only needs to retain the initiator for the duration of visitProposedToLocation -- to ensure that any calls to startVisit pass the initiator to the Visit -- then I wonder if you could do it with a wrapper around that method call. Something along the lines of this (and this is just pseudocode so excuse my typos :)):

  proposeVisit(location: URL, options: Partial<VisitOptions> = {}) {
        ...

        this.withCurrentInitiator(options.initiator, () => {
          this.delegate.visitProposedToLocation(location, options)
        })

        ...
    }
  }

  startVisit(locatable: Locatable, restorationIdentifier: string, options: Partial<VisitOptions> = {}) {
    ...

    this.currentVisit = new Visit(this, expandURL(locatable), restorationIdentifier, {
      referrer: this.location,
      initiator: this.currentInitiator,
      ...options,
    })
    this.currentVisit.start()
  }

  // Helpers

  withCurrentInitiator(initiator: Element | undefined, func: () => void) {
    this.currentInitiator = initiator
    func()
    delete this.currentInitiator
  }

So that way, any startVisit calls while handling a visitProposedToLocation will implicitly pass along the initiator to the Visit. It looks like that would satisfy the browser adapter and the iOS adapter And combined with your code to remove the initiator from the options before calling the adapter, it would avoid the crash.

how do we make sure that the visit is created with the correct initiator?

That's the bit that worries me :) I'm just not sure what the edge cases might be here. If the only downside was that visits started outside of a visitProposedToLocation would need to explicitly set their own initiator or use the default documentElement then that might be fine. But I'm not familiar enough with this code to know what else might happen.

[domchristie]

I wonder if you could do it with a wrapper around that method call.

Nice! ✨

how do we make sure that the visit is created with the correct initiator? :)

Perhaps we could do the following:

1. store the element along with a uuid (initiator = {id, element})
2. pass the uuid through to the adapter
3. in in navigator#startVisit, look up the initiator by uuid—use it if it exists, otherwise ignore?

Or that that a little over-engineered?

Also, I'm not really certain that Navigator is the right place to store the initiator. Given that it calls stop in startVisit, it feels like it's designed to handle one visit at a time, rather than storing data across vists. Perhaps on the History? 🤔