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

Fix compatibility shim for mobile apps #372

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

okcomputer93
Copy link

This fixes an issue in the suggested compatibility shim for mobile apps,
when the adapter is handled by turbolinks-ios or turbolinks-android.
Because the reference to adapter.visitStarted is stored as a
variable, when it's called again the reference to this is not pointing
to the adapter's this but to window.

Co-authored-by: Juan Zenón [email protected]

This fixes an issue in the suggested compatibility shim for mobile apps,
when the adapter is handled by turbolinks-ios or turbolinks-android.
Because the reference to `adapter.visitStarted` is  stored as a
variable, when it's called again the reference to `this` is not pointing
to the adapter's `this` but to `window`.

Co-authored-by: Juan Zenón <[email protected]>
@@ -135,7 +135,7 @@ window.Turbolinks = {

// Old mobile adapters use visit.location.absoluteURL, which is not available
// because Turbo dropped the Location class in favor of the DOM URL API
const adapterVisitStarted = adapter.visitStarted
const adapterVisitStarted = adapter.visitStarted.bind(adapter)
Copy link
Author

Choose a reason for hiding this comment

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

We're having this issue while trying to migrate to Turbo from our apps in turbolinks-ios,when the visitStarted method in the adapter is called, this is a reference to window instead of the adapter itself:
https://github.com/turbolinks/turbolinks-ios/blob/1c891b9ffbb89666a05b7c5b02a8fdd700e92323/Turbolinks/WebView.js#L60-L63

@okcomputer93 okcomputer93 marked this pull request as ready for review August 19, 2022 23:59
@dhh
Copy link
Member

dhh commented Sep 13, 2022

cc @jayohms

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