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

Get web view's URL at runtime #24

Merged
merged 10 commits into from
Mar 1, 2024
Merged

Get web view's URL at runtime #24

merged 10 commits into from
Mar 1, 2024

Conversation

joemasilotti
Copy link
Member

If a Turbo request is redirected to a page with a Strada component this check would always fail. location is only set when BridgeDelegate is initialized. This change dynamically grabs the URL from the web view, falling back to the original location if nil.

Fixes #23 and should also address part of #19.

If a Turbo request is redirected to a page with a Strada component this
check would always fail. `location` is only set when `BridgeDelegate` is
initialized. This change dynamically grabs the URL from the web view,
falling back to the original `location` if nil.

Fixes #23
Copy link
Collaborator

@svara svara 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 nice solution @joemasilotti!
I've left some feedback in #25.

Tests/MessageTests.swift Show resolved Hide resolved
Source/BridgeDelegate.swift Outdated Show resolved Hide resolved
Copy link
Collaborator

@jayohms jayohms 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 fine solution 👍

I still believe this is not a core Strada issue, but a combination of redirect urls and Turbo Frames not proposing visits to the mobile Turbo adapters. A ViewController should not have its webview url change from underneath it — because that bypasses the path configuration, and the url maybe be configured a different ViewController or other path properties.

But, this added flexibility is fine and I don't anticipate any issues.

@joemasilotti
Copy link
Member Author

I still believe this is not a core Strada issue, but a combination of redirect urls and Turbo Frames not proposing visits to the mobile Turbo adapters. A ViewController should not have its webview url change from underneath it — because that bypasses the path configuration, and the url maybe be configured a different ViewController or other path properties.

Perhaps a tangent but how do you see this working in an ideal world? If a page redirects should a new visit be proposed? What happens to the "old" screen in that scenario, especially if the new one should be a modal?

@jayohms
Copy link
Collaborator

jayohms commented Dec 13, 2023

Perhaps a tangent but how do you see this working in an ideal world? If a page redirects should a new visit be proposed?

Ideally yes — but it seems as though lots of apps are using redirects to load the root view controller, so this could make a pretty undesirable experience. I recommend not using a redirect to load the initial page for apps.

What happens to the "old" screen in that scenario, especially if the new one should be a modal?

It's an application bug if you use a redirect for your (non-modal) root view controller to a modal :)

@joemasilotti
Copy link
Member Author

It's an application bug if you use a redirect for your (non-modal) root view controller to a modal :)

Good to know! I've had a bunch of folks asking me about this exact scenario. Usually related to loading /dashboard or similar and then needing to show authentication.

Do you have a recommendation for a workaround there? Assuming folks are not using native authentication.

@obromios
Copy link

I tried this PR on issue#28 and it appears to fix the problem.

@joemasilotti joemasilotti merged commit a44a86d into main Mar 1, 2024
1 check passed
@joemasilotti joemasilotti deleted the fix-redirect-issue branch March 1, 2024 17:53
@dginsburg
Copy link
Contributor

dginsburg commented Mar 6, 2024

Perhaps a tangent but how do you see this working in an ideal world? If a page redirects should a new visit be proposed?

Ideally yes — but it seems as though lots of apps are using redirects to load the root view controller, so this could make a pretty undesirable experience. I recommend not using a redirect to load the initial page for apps.

@jayohms, I'm here because we ran into the issue in this ticket when using Turbo Frames and Strada. I'd say the problem is broader than your example. It's not just redirects and it's not just a root view controller in the nav stack. Any page that uses redirects or uses Turbo Frames (unsure about morphing) to change the location without a new Visit can hit these issues in the Hotwire iOS native adapters.

Turbo iOS has a similar issue
https://github.com/hotwired/turbo-ios/blob/main/Source/Visitable/VisitableViewController.swift#L6-L11

In Turbo iOS you can work around it because visitableURL is an open var. But I think the default is harmful. The redirects and this way of using Turbo Frames aren't patterns that 37s tends to use, so it's understandable that it never came up as a problem. But they are common patterns in my experience and reasonable ways to use Rails/Hotwire.

The core of the problem is that the location of a page can change after the native controller is created with the original URL. Hotwire allows that so I think it's a first class use case that the native adapters should handle by default.

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.

When the initial page redirects, the BridgeDelegate.location becomes stale
5 participants