Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent errors when communicating with iOS webview #733

Closed
wants to merge 5 commits into from

Conversation

domchristie
Copy link
Contributor

@domchristie domchristie commented Sep 23, 2022

To successfully communicate with the iOS view, messages need to be of a type supported by the structuredClone algorithm. The new TransferableVisitOptions type ensures that VisitOptions passed to the adapter conform to this type. The StructuredCloneValue type has been created based on https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm#supported_types

This should hopefully prevent future errors such as #720


An alternative to this might be that we become stricter with the VisitOptions we pass around, and instead of using Partial use Pick and Omit?

/cc @kevinmcconnell

Copy link
Collaborator

@kevinmcconnell kevinmcconnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great idea! 👏 I really like the idea of using the compiler to enforce the safety of the options fields, rather than rely on a comment (or our memory).

My only thought (as mentioned below) is that I'd be tempted to tweak it so that adding a non-transferable option would cause a compiler error, rather than it being automatically sanitised. It just makes it more obvious to the person adding that field that that's happening.

src/core/session.ts Outdated Show resolved Hide resolved
src/core/types.ts Show resolved Hide resolved
src/core/drive/visit.ts Outdated Show resolved Hide resolved
@domchristie
Copy link
Contributor Author

Just to add: this doesn't change the existing behaviour; it just adds type checking, and removes any properties that would cause a DataCloneError

To successfully communicate with the iOS view, messages need to be of a type supported by the structuredClone algorithm.
The new TransferableVisitOptions type ensures that VisitOptions passed to the adapter conform to this type. The StructuredCloneValue type has been created based on https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm#supported_types
@domchristie
Copy link
Contributor Author

(And I'm not sure why the tests are failing 🤷‍♂️ They also fail for me locally on the main branch`)

@domchristie
Copy link
Contributor Author

Just to add: this doesn't change the existing behaviour; it just adds type checking, and removes any properties that would cause a DataCloneError
(And I'm not sure why the tests are failing 🤷‍♂️ They also fail for me locally on the main branch`)

On second thoughts, this does alter the behaviour, because the options passed to Navigator#startVisit will be without referrer and visitCachedSnapshot. This is amended in 122fd26

@domchristie
Copy link
Contributor Author

Superseded by #750

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

Successfully merging this pull request may close these issues.

2 participants