Move cache snapshot loading to Visit#start()

[afcapel]

This fixes a problem introduced when the cache interface changed in #949

Since the cache can be loaded from disk using the browser Cache API, it is possible that the cache snapshot will be loaded asynchronously. This means that the calls to load cache snapshots need to be awaited.

Because of that we also changed visit.hasCachedSnapshot() to be async and return a Promise. However, hasCachedSnapshot is used in the iOS adapter and serialized into and data object we send to the native side via postMessage. When postMessage receives a Promise instead of a boolean value it fails with a DataCloneError because it can't serialize the Promise.

This commit moves the cache snapshot loading to Visit#start() and stores the result in a cachedSnapshot property. This allows us to keep the hasCachedSnapshot() method synchronous and return a boolean value.

It means that Visit.start is now async, but I haven't found any callers that rely on it being synchronous.

[domchristie]

However, hasCachedSnapshot is used in the iOS adapter and serialized into and data object we send to the native side via postMessage. When postMessage receives a Promise instead of a boolean value it fails with a DataCloneError because it can't serialize the Promise.

This happened before (#720). Should we test that the object sent to the native adapters conforms to the structured clone algorithm?

[jayohms]

Good call @domchristie, I added a unit test to check that visit.hasCachedSnapshot() returns a boolean value. And I confirmed it would have failed before this change.

[jayohms]

Thank you, looks good to me @afcapel 👍

[afcapel]

@domchristie agreed, this is a recurrent problem. It's too easy to make a mistake here 👍

Should we test that the object sent to the native adapters conforms to the structured clone algorithm?

How would that work? Do we add that to the iOS and Android adapters?

[domchristie]

@afcapel I had a quick look into adding some unit tests on this repo to make sure that anything sent to the native adapter methods doesn't raise an error when passed to structuredClone (particularly Visit options, where it's inviting to add more). However it didn't look straightforward.

@kevinmcconnell and I discussed some ideas in #720, and it was suggested that the ideal solution would be something on the native adapter side, but backwards compatibility was an issue. (Perhaps less so with a Turbo 8 major version change?)

I implemented @kevinmcconnell's idea of adding a withTransferableVisitOptions, but it will just filter out non-transferable values, and would've quietly failed in the above case.

I am not a fan of types (and don't wish to bring up that debate again), but I think this might be a good place for them. In an earlier PR I added a StructuredCloneValue type, which might be useful to apply for the adapter methods. (Just suggesting this for possible inspiration.)

So there's my thoughts! Not sure what the best approach is really! :/