[go_router] Make `queryParams` a `Map<String, dynamic>` #2392

ValentinVignal · 2022-07-27T10:01:37Z

fixes flutter/flutter#108390

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…t-of-query-parameters # Conflicts: # packages/go_router/CHANGELOG.md

chunhtai

code looks good, just need a bit more test.

chunhtai · 2022-08-02T19:42:10Z

packages/go_router/test/parser_test.dart

    expect(configuration.namedLocation('SNAKE_CASE'), '/hij?');
  });

+  test(


Can you add a test in go_router_test.dart to test the entire flow from context.goNamed() to the actual page is built with the correct query params?

Thanks for the suggestion, while writing the test, I actually faced an issue/bug:

If you run this code snippet:

final uri = Uri(path: 'a/b/', queryParameters: {'q1': 'v1', 'q2': ['v2', 'v3']}); print(uri.queryParametersAll); print(uri.queryParameters);

It will log

{q1: [v1], q2: [v2, v3]} {q1: v1, q2: v3}

Because of that, only carrying around queryParams is not enough, I had to add queryParametersAll everywhere.

Do you want me to remove queryParams as the data is now kind of duplicated? This would be a breaking change I think.
Or I could create getters Map<String, dynamic> get queryParams to not duplicate stored data and not introduce a breaking change?

What do you think?

…eters # Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/pubspec.yaml

…Named [skip ci]

chunhtai

Can you fix the merge error and merge conflict ? also the build need to pass before this can merge

chunhtai · 2022-08-03T15:11:47Z

packages/go_router/CHANGELOG.md


 - Allows `Map<String, dynamic>` maps as `queryParams` of `goNamed`, `replacedName`, `pushNamed` and `namedLocation`. 

+## 4.2.5


This seems to be merging error?

I fixed the merge conflict in 13e6cb1

I'm working on fixing go_router_builder

…eters # Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/pubspec.yaml

… Uri

ValentinVignal · 2022-08-04T16:41:01Z

@chunhtai I should have fixed the linter now

NazarenoCavazzon · 2022-08-04T23:44:21Z

Good idea, in my job when we need to give a widget a parameter that's not a String we use extra as a Map to pass the variables. This makes a lot of sense.

…t-of-query-parameters # Conflicts: # packages/go_router/CHANGELOG.md

ValentinVignal · 2022-08-17T05:01:47Z

Hello @chunhtai , is there something else I should do about this PR ?

chunhtai

LGTM, just some nits

chunhtai · 2022-08-17T16:33:52Z

packages/go_router/lib/src/match.dart

  /// Query parameters for the matched route.
  final Map<String, String> queryParams;

+  /// Query parameters for the matched route.


Can you add a bit more explanation on whats different between this and queryParams?

I tried to write a better documentation in d9f9315

Tell me if there is something to change

chunhtai · 2022-08-17T16:45:11Z

packages/go_router/lib/src/matching.dart

      routes: configuration.routes,
      parentFullpath: '',
      parentSubloc: '',
+      // Here, if the request is `a/b/?q1=v1&q2=v2&q2=v3`;


This comment feel a bit out of place, maybe put this is doc string of the class property instead?

chunhtai · 2022-08-17T16:45:38Z

packages/go_router/test/go_router_test.dart

      final GoRouter router = await createRouter(routes, tester);
      router.go('/login/');
      final List<RouteMatch> matches = router.routerDelegate.matches.matches;
-      print(matches);


nice catch!

johnpryan · 2022-08-17T17:07:08Z

packages/go_router/CHANGELOG.md

@@ -1,5 +1,6 @@
-## NEXT
+## 4.2.8


Shouldn't this be 4.3.0 since this adds a new parameter to GoRouterState?

I changed the version number in e722483

johnpryan · 2022-08-17T17:09:56Z

packages/go_router/test/go_router_test.dart

+
+      final GoRouter router = await createRouter(routes, tester);
+
+      router.goNamed('page', queryParams: const <String, dynamic>{


Can we add a test for go() too?

Yep, I added a test in d9f9315

johnpryan · 2022-08-17T17:13:37Z

Just confirming, there's still work to do in go_router_builder (flutter/flutter#108437), but this won't break it?

…t-of-query-parameters

…rsAll

ValentinVignal · 2022-08-18T01:35:41Z

Just confirming, there's still work to do in go_router_builder (flutter/flutter#108437), but this won't break it?

Hum, I might be wrong but I think we are good.
Here we are

Allowing the user to pass Map<String, dynamic> instead of Map<String, String> to goNamed. But goNamed is not used by the generated code of go_router_builder (I think). It generates the location itself with the queryParameters (using Uri) and uses .go() and push().
Adding a new getter/parameter queryParametersAll but I didn't modify anything for queryParams so the code generated by go_router_builder (_fromState) should still be able to use it.

But yeah, in order to do flutter/flutter#108437, we should merge this PR first, so GoRouter can support multiple instances of the same key inside the url. Then go_router_builder can be changed to support Iterables.

…eters # Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/pubspec.yaml

…t-of-query-parameters # Conflicts: # packages/go_router/CHANGELOG.md

ValentinVignal · 2022-08-24T01:10:09Z

@johnpryan Do you want me to do anything else on this PR?

chunhtai · 2022-08-25T21:58:38Z

cc @johnpryan

…t-of-query-parameters # Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/pubspec.yaml # packages/go_router/test/test_helpers.dart

packages/go_router/lib/src/configuration.dart

loic-sharma · 2022-08-26T20:52:43Z

@chunhtai Isn't this technically a breaking change since we changed method signatures? Should we target the v5 branch instead?

chunhtai · 2022-08-26T21:50:58Z

@loic-sharma I don't think this will be breaking you can upward cast type in map without addition code.
This should work fine.

Map<String, dynamic> queryParams = <String, String>{};

Or is there a use case I may be missing?

loic-sharma · 2022-08-26T22:13:20Z

Yup never mind my concern. This works as expected too:

class Test {
  void go(Map<String, dynamic> queryParams) {
    print(queryParams);
  }
}

typedef Go = void Function(Map<String, String> queryParams);

void main() async {
  var test = Test();
  Go go = test.go;
  go({'foo': 'bar'});
}

Sotatek-PhiPham · 2022-09-13T09:43:01Z

i see you already support map<String, dynamic> but queryParams in GoRouterState is still map<String, String> so i can't get data, it throws type 'SomethingLikeEnum' is not a subtype of type 'Iterable'
GoRoute( name: AppRouterName.resendEmailPage, path: '/${AppRouterName.resendEmailPage}', builder: (context, state) { SomethingLikeEnum data = state.queryParams["data"] as SomethingLikeEnum; return ResendEmailPage(data: data); }, ),

ValentinVignal · 2022-09-13T14:43:22Z

@Sotatek-PhiPham You can use GoRouterState.queryParametersAll which is Map<String, dynamic>

Sotatek-PhiPham · 2022-09-13T14:57:48Z

i tried queryParametersAll but it is Map<String, List>. I am using go router version 4.4.1

ValentinVignal · 2022-09-13T15:43:51Z

Yes, it uses Uri and this is how it works:

Uri.parse() takes Map<String, dynamic>? queryParameters

And Uri's getters are:

Map<String, String> queryParameters
Map<String, List<String>> queryParametersAll

vrman · 2023-06-02T23:11:16Z

still cant get bool in queryParameters , i need bool.parse to convert string to bool , is this possible to make queryParameters getter to <String, dynamic> ?

chunhtai · 2023-06-02T23:14:19Z

the dynamic is not meant to be use for various type. It is to support list query parameter. https://api.flutter.dev/flutter/dart-core/Uri/queryParametersAll.html

locking this issue

ValentinVignal added 4 commits July 27, 2022 17:56

🏷️ Allow Map<String, dynamic> for queryParams

f753c09

✅ Update the tests

5bba064

📝 Update the version and changelog

7a7b36f

Merge remote-tracking branch 'upstream/main' into go-router/allow-lis…

f6937ad

…t-of-query-parameters # Conflicts: # packages/go_router/CHANGELOG.md

github-actions bot added the p: go_router label Jul 27, 2022

🔀 Fix merge conflict

3ccf5e8

stuartmorgan-g requested a review from chunhtai August 2, 2022 18:48

chunhtai reviewed Aug 2, 2022

View reviewed changes

ValentinVignal added 4 commits August 3, 2022 09:10

Merge tag 'go_router-v4.2.5' into go-router/allow-list-of-query-param…

a66bc4b

…eters # Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/pubspec.yaml

🧪 Failing test testing the query parameters are correctly set with go…

174b732

…Named [skip ci]

🔦 Show where uri.queryParameters is used [skip ci]

a9fe262

🐛 Add queryParametersAll everywhere

a13a58d

chunhtai reviewed Aug 3, 2022

View reviewed changes

ValentinVignal added 3 commits August 4, 2022 09:11

Merge tag 'go_router-v4.2.7' into go-router/allow-list-of-query-param…

13e6cb1

…eters # Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/pubspec.yaml

🏷️ Match types of queryParams and queryParameters with the types from…

db13fab

… Uri

🏷️ Change dynamic to String/List<String> when it comes from Uri

4c6aba7

ValentinVignal requested a review from chunhtai August 4, 2022 16:40

Merge remote-tracking branch 'upstream/main' into go-router/allow-lis…

63db296

…t-of-query-parameters # Conflicts: # packages/go_router/CHANGELOG.md

chunhtai approved these changes Aug 17, 2022

View reviewed changes

chunhtai requested a review from johnpryan August 17, 2022 16:49

johnpryan requested changes Aug 17, 2022

View reviewed changes

ValentinVignal added 4 commits August 18, 2022 09:07

Merge remote-tracking branch 'upstream/main' into go-router/allow-lis…

3556e1e

…t-of-query-parameters

⬆️ Change the version number to 4.3.0

e722483

📝 Explain what is the different between queryParams and queryParamete…

d9f9315

…rsAll

✅ Add a test for go()

74fd75a

ValentinVignal requested a review from johnpryan August 18, 2022 01:35

ValentinVignal added 3 commits August 19, 2022 09:09

Merge tag 'go_router-v4.2.8' into go-router/allow-list-of-query-param…

142a7dd

…eters # Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/pubspec.yaml

🔥 Remove duplicated line in changelog

923f005

Merge remote-tracking branch 'upstream/main' into go-router/allow-lis…

9c7987d

…t-of-query-parameters # Conflicts: # packages/go_router/CHANGELOG.md

Merge remote-tracking branch 'upstream/main' into go-router/allow-lis…

80e8ddb

…t-of-query-parameters # Conflicts: # packages/go_router/CHANGELOG.md # packages/go_router/pubspec.yaml # packages/go_router/test/test_helpers.dart

loic-sharma reviewed Aug 26, 2022

View reviewed changes

packages/go_router/lib/src/configuration.dart Show resolved Hide resolved

loic-sharma approved these changes Aug 26, 2022

View reviewed changes

johnpryan approved these changes Aug 26, 2022

View reviewed changes

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 26, 2022

auto-submit bot merged commit a95e400 into flutter:main Aug 26, 2022

ValentinVignal deleted the go-router/allow-list-of-query-parameters branch August 27, 2022 07:08

flutter locked and limited conversation to collaborators Jun 2, 2023


		- Allows `Map<String, dynamic>` maps as `queryParams` of `goNamed`, `replacedName`, `pushNamed` and `namedLocation`.

		## 4.2.5


		final GoRouter router = await createRouter(routes, tester);

		router.goNamed('page', queryParams: const <String, dynamic>{

[go_router] Make queryParams a Map<String, dynamic> #2392

[go_router] Make queryParams a Map<String, dynamic> #2392

Uh oh!

Conversation

ValentinVignal commented Jul 27, 2022 • edited by chunhtai Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ValentinVignal Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ValentinVignal commented Aug 4, 2022

Uh oh!

NazarenoCavazzon commented Aug 4, 2022

Uh oh!

ValentinVignal commented Aug 17, 2022

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnpryan commented Aug 17, 2022

Uh oh!

ValentinVignal commented Aug 18, 2022

Uh oh!

ValentinVignal commented Aug 24, 2022

Uh oh!

chunhtai commented Aug 25, 2022

Uh oh!

Uh oh!

loic-sharma commented Aug 26, 2022

Uh oh!

chunhtai commented Aug 26, 2022

Uh oh!

loic-sharma commented Aug 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sotatek-PhiPham commented Sep 13, 2022

Uh oh!

ValentinVignal commented Sep 13, 2022

Uh oh!

Sotatek-PhiPham commented Sep 13, 2022

Uh oh!

ValentinVignal commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vrman commented Jun 2, 2023

Uh oh!

chunhtai commented Jun 2, 2023

Uh oh!

Reviewers

Assignees

[go_router] Make `queryParams` a `Map<String, dynamic>` #2392

[go_router] Make `queryParams` a `Map<String, dynamic>` #2392

ValentinVignal commented Jul 27, 2022 •

edited by chunhtai

Loading

ValentinVignal Aug 4, 2022 •

edited

Loading

loic-sharma commented Aug 26, 2022 •

edited

Loading

ValentinVignal commented Sep 13, 2022 •

edited

Loading