Skip to content

Conversation

@AhmedLSayed9
Copy link
Contributor

This PR updates examples to use uri.path instead of uri.toString() for accessing the current location.

While the examples don't use deep linking, promoting the usage of uri.toString() in the examples doesn't seem to be a good idea as it can lead to issues when it's used with deep links (It'll include host and scheme).

Pre-launch Checklist

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

Good call. This change looks good to me, can you also bump package version and change log?

@chunhtai
Copy link
Contributor

chunhtai commented Apr 8, 2024

Some of the tests are failing. can you fix them as well?

@AhmedLSayed9 AhmedLSayed9 force-pushed the update_location_access branch from db04931 to 05dfb08 Compare April 8, 2024 17:41
@AhmedLSayed9
Copy link
Contributor Author

AhmedLSayed9 commented Apr 8, 2024

Some of the tests are failing. can you fix them as well?

When passing a query parameter, i.e doing: router.go('/?p=123'):

  • GoRouterState.of(context).uri.path result in: /
  • GoRouterState.of(context).matchedLocation result in: /
  • GoRouterState.of(context).uri result in: /?p=123

So, accessing current location using uri.path will not include query params. Is that ok?

If that's fine, I can fix the tests by checking for GoRouterState.of(context).uri.queryParameters individually.

@AhmedLSayed9
Copy link
Contributor Author

@chunhtai
I'm waiting for your answer before going further with fixing the tests :)

@chunhtai
Copy link
Contributor

chunhtai commented Apr 10, 2024

I think you can just change the expect to ignore query parameter since now the example does not print query parameter onto the page.

If that's fine, I can fix the tests by checking for GoRouterState.of(context).uri.queryParameters individually.

I am fine with it

@AhmedLSayed9 AhmedLSayed9 force-pushed the update_location_access branch from adfb7ae to ab6f793 Compare April 10, 2024 20:23
@AhmedLSayed9
Copy link
Contributor Author

@chunhtai Can you check now?

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

LGTM

@chunhtai chunhtai requested a review from hannah-hyj April 10, 2024 21:27
Copy link
Member

@hannah-hyj hannah-hyj left a comment

Choose a reason for hiding this comment

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

LGTM

@chunhtai
Copy link
Contributor

looks like there are still some ci error

@AhmedLSayed9
Copy link
Contributor Author

@chunhtai
There was a failing test for all_types example, I've fixed it.

@hannah-hyj hannah-hyj added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 11, 2024
@auto-submit auto-submit bot merged commit 488da36 into flutter:main Apr 11, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Apr 12, 2024
flutter/packages@e98839a...78f684c

2024-04-12 [email protected] Roll Flutter from 557fbf5 to 53cba24 (11 revisions) (flutter/packages#6509)
2024-04-11 [email protected] [file_selector] Remove OCMock from iOS implementation (flutter/packages#6503)
2024-04-11 [email protected] Access current location using uri.path to support deep links (flutter/packages#6474)
2024-04-11 [email protected] Roll Flutter from 97cd47a to 557fbf5 (22 revisions) (flutter/packages#6502)
2024-04-11 [email protected] [packages] Set parallelizable to NO to reduce test flakiness in packages project tests (flutter/packages#6471)
2024-04-11 [email protected] Update multiple packages to depend on versions with iOS privacy manifest included (flutter/packages#6355)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
gilnobrega pushed a commit to gilnobrega/flutter that referenced this pull request Apr 22, 2024
flutter/packages@e98839a...78f684c

2024-04-12 [email protected] Roll Flutter from 557fbf5 to 53cba24 (11 revisions) (flutter/packages#6509)
2024-04-11 [email protected] [file_selector] Remove OCMock from iOS implementation (flutter/packages#6503)
2024-04-11 [email protected] Access current location using uri.path to support deep links (flutter/packages#6474)
2024-04-11 [email protected] Roll Flutter from 97cd47a to 557fbf5 (22 revisions) (flutter/packages#6502)
2024-04-11 [email protected] [packages] Set parallelizable to NO to reduce test flakiness in packages project tests (flutter/packages#6471)
2024-04-11 [email protected] Update multiple packages to depend on versions with iOS privacy manifest included (flutter/packages#6355)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
TecHaxter pushed a commit to TecHaxter/flutter_packages that referenced this pull request May 22, 2024
…#6474)

This PR updates examples to use `uri.path` instead of `uri.toString()` for accessing the current location.

While the examples don't use deep linking, promoting the usage of `uri.toString()` in the examples doesn't seem to be a good idea as it can lead to issues when it's used with deep links (It'll include host and scheme).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App p: go_router_builder p: go_router

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants