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 #190

Merged
merged 9 commits into from
Mar 22, 2024

Conversation

leonvogt
Copy link
Contributor

@leonvogt leonvogt commented Mar 5, 2024

closes #188

@joemasilotti
Copy link
Member

@jayohms, this is ready for review now having moved to a global configuration, only.

I'd love your feedback on the naming of the configuration option. I nested it under a new sub-class which makes sense to me:

Turbo.config.pathConfiguration.matchQuery = true

But I'm open to keeping it one level deep and using:

Turbo.config.matchPathConfigurationQuery = true

Or something similar/cleaner.

@joemasilotti joemasilotti self-requested a review March 13, 2024 22:43
@leonvogt
Copy link
Contributor Author

Thanks a lot for your commits @joemasilotti!
Looks a lot better now!

One question though:
On the Android side the query parameters are beeing checked by default.
With the current implementation of this PR, they are not being checked by default. As a result, users would need to manually update the global configuration to align with the same behavior as Turbo Android.

Would you be open to the idea of enabling this feature by default to align the behavior?
I understand that it might introduce some unwanted side effects for users. However, if we declare it as a potential breaking change in the release notes, users who prefer to maintain the old behavior could simply update the newly introduced global configuration.

What are your thoughts on this?

Docs/PathConfiguration.md Outdated Show resolved Hide resolved
Docs/PathConfiguration.md Outdated Show resolved Hide resolved
By default, the path configuration only looks at the path component of the URL. Enable query matching via:

```swift
Turbo.config.pathConfiguration.matchQuery = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Turbo.config.pathConfiguration.matchQuery = true
Turbo.config.pathConfiguration.matchQueryStrings = true

Docs/PathConfiguration.md Outdated Show resolved Hide resolved
public func properties(for url: URL) -> PathProperties {
properties(for: url.path)
if Turbo.config.pathConfiguration.matchQuery, let query = url.query {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if Turbo.config.pathConfiguration.matchQuery, let query = url.query {
if Turbo.config.pathConfiguration.matchQueryStrings, let query = url.query {


public extension TurboConfig {
class PathConfiguration {
public var matchQuery = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public var matchQuery = false
public var matchQueryStrings = false

@jayohms
Copy link
Collaborator

jayohms commented Mar 22, 2024

One question though:
On the Android side the query parameters are beeing checked by default.
With the current implementation of this PR, they are not being checked by default. As a result, users would need to manually update the global configuration to align with the same behavior as Turbo Android.

Would you be open to the idea of enabling this feature by default to align the behavior?
I understand that it might introduce some unwanted side effects for users. However, if we declare it as a potential breaking change in the release notes, users who prefer to maintain the old behavior could simply update the newly introduced global configuration.

What are your thoughts on this?

I'd say the only reason we're adding the new configuration option is so we avoid breaking existing apps. Otherwise, I'd be in favor of always matching patterns against path + query strings. So, let's avoid breaking apps on both platforms and keep the default behaviors in place.

We have upcoming changes later this year to make make the libraries easier to use out-of-the-box with sane defaults, so that'll be a good time to reevaluate the defaults.

@jayohms jayohms merged commit 29414c5 into hotwired:main Mar 22, 2024
joemasilotti added a commit to hotwired/hotwire-native-ios that referenced this pull request Mar 23, 2024
@leonvogt leonvogt deleted the add-url-params-check branch March 26, 2024 12:56
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.

Add support for query parameters in the PathConfiguration
3 participants