-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[go_router] Adds caseSensitive
to GoRoute
#8992
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
[go_router] Adds caseSensitive
to GoRoute
#8992
Conversation
@@ -275,6 +275,7 @@ class GoRoute extends RouteBase { | |||
super.parentNavigatorKey, | |||
super.redirect, | |||
this.onExit, | |||
this.caseSensitive = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chunhtai, from your comment flutter/flutter#162487 (comment),
based on https://www.w3.org/TR/WD-html40-970708/htmlweb.html, the URL should be case sensitive
I prefer to set the default to `true, ' but that would be a breaking change. I'm fine writing a migration doc
What do you prefer me to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree we should probably default it to true to provide the correct behavior by default, but yeah we should bump major version and a breaking change doc if we do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok ! Will do !
Converting as draft while the design doc is being written and the code updated |
Design document has been created: flutter/website#11867 |
@chunhtai The URL is now case sensitive by default |
Adds the migration guide for go_router v15 (See flutter/packages#8992, and more precisely flutter/packages#8992 (comment)) ## Presubmit checklist - [x] This PR is marked as draft with an explanation if not meant to land until a future stable release. - [x] This PR doesn’t contain automatically generated corrections (Grammarly or similar). - [x] This PR follows the [Google Developer Documentation Style Guidelines](https://developers.google.com/style) — for example, it doesn’t use _i.e._ or _e.g._, and it avoids _I_ and _we_ (first person). - [x] This PR uses [semantic line breaks](https://github.com/dart-lang/site-shared/blob/main/doc/writing-for-dart-and-flutter-websites.md#semantic-line-breaks) of 80 characters or fewer.
The migration guide has been merged and is now available at the go url |
e4aff26
to
ca1432f
Compare
@@ -218,7 +218,17 @@ abstract class RouteMatchBase with Diagnosticable { | |||
final String newMatchedLocation = | |||
concatenatePaths(matchedLocation, pathLoc); | |||
final String newMatchedPath = concatenatePaths(matchedPath, route.path); | |||
if (newMatchedLocation.toLowerCase() == uri.path.toLowerCase()) { | |||
|
|||
final String caseSensitiveNewMatchedLocation; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the new name is more confusing than the old name, because it can contains case insensitive string in the else case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, What do you think of newMatchedLocationToCompare
and uriPathToCompare
? I added them in Add better name and documentation
@@ -437,6 +438,9 @@ class GoRoute extends RouteBase { | |||
/// ``` | |||
final ExitCallback? onExit; | |||
|
|||
/// Whether the path is case sensitive or not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add more description? perhaps with some example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I added more in Add better name and documentation, tell me if you need more
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for fixing this!
autosubmit label was removed for flutter/packages/8992, because - The status or check suite Mac_arm64 custom_package_tests stable has failed. Please fix the issues identified (or deflake) before re-applying this label. |
48ba286
to
12f0136
Compare
Fixes flutter/flutter#162487
Fixes flutter/flutter#166576
Pre-Review Checklist
[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3