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

Add support for query parameters in the PathConfiguration #188

Closed
leonvogt opened this issue Mar 4, 2024 · 3 comments · Fixed by #190
Closed

Add support for query parameters in the PathConfiguration #188

leonvogt opened this issue Mar 4, 2024 · 3 comments · Fixed by #190

Comments

@leonvogt
Copy link
Contributor

leonvogt commented Mar 4, 2024

Currently, a PathConfiguration that captures certain URL parameters, such as:

{
  patterns: [".*\\?open_in_external_browser=true.*"],
  properties: {open_in_external_browser: true}
}

works out of the box in Turbo-Android but not in Turbo-iOS.
There is a comment in the properties implementation that says query parameters are not supported yet but likely will be in the future.
Is there a technical reason why this was not implemented yet?
Before creating a PR, I wanted ask what the reasons were for this decision and what we need to consider when implementing this feature.
It would be nice if Turbo-iOS and Turbo-Android would behave the same way in this regard.
Thanks!

@jayohms
Copy link
Collaborator

jayohms commented Mar 4, 2024

It would be nice if Turbo-iOS and Turbo-Android would behave the same way in this regard.

100%. There's no technical reason why they're different — except oversight. Let's align them. Since this is a potential change in behavior for existing path configuration files, we'll likely need to add a new configuration option to maintain the current behavior.

@joemasilotti
Copy link
Member

@leonvogt, let me know if you're open to submitting a PR. If not, I'm happy to take a pass myself.

@leonvogt
Copy link
Contributor Author

leonvogt commented Mar 5, 2024

@jayohms thanks for the fast reply!
I'm not quite sure I understand your suggestion with the configuration option.
To maintain the current behavior, I would introduce an optional parameter to the properties function, that will enable the new behavior.
People who don't want the new behavior can use this parameter to opt out of this change.
I created a small PR to demonstrate this idea: #190

@joemasilotti if this PR goes in a different direction than you intended, I'm happy to close it in favor of your approach.

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

Successfully merging a pull request may close this issue.

3 participants