[go_router] #127016 preserved route name case when caching `_nameToPath` #4196

Khalidm98 · 2023-06-12T14:02:06Z

preserved route name case when caching _nameToPath by not using toLowerCase()

Related issue:
#127016

…sing `toLowerCase()``

flutter-dashboard · 2023-06-12T14:02:09Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

chunhtai

Can you write a test to make sure we don't break it accidentally?

chunhtai · 2023-06-12T16:30:24Z

packages/go_router/CHANGELOG.md

+- Preserves route name case when caching `_nameToPath` by not using `toLowerCase()`.<br>
+this is useful to make route names case sensitive


Suggested change

- Preserves route name case when caching `_nameToPath` by not using `toLowerCase()`.<br>

this is useful to make route names case sensitive

- Makes namedLocation and route name related APIs case sensitive.

sure, will write the test

I added the test and modified the changelog as suggested.
Do you have any other comments or suggestions in mind?
Thanks ✌️

chunhtai

LGTM, some suggestions on test

chunhtai · 2023-06-12T22:14:49Z

packages/go_router/test/name_case_test.dart

+      );
+
+      // go to ScreenB
+      navKey.currentContext!.goNamed('name');


Suggested change

navKey.currentContext!.goNamed('name');

router.goNamed('name');

chunhtai · 2023-06-12T22:15:53Z

packages/go_router/test/name_case_test.dart

+
+      // go to ScreenB
+      navKey.currentContext!.goNamed('name');
+      assert(GoRouter.of(navKey.currentContext!).location == '/path');


Suggested change

assert(GoRouter.of(navKey.currentContext!).location == '/path');

expect(find.byType(ScreenB), findsOneWidget);

chunhtai · 2023-06-12T22:16:09Z

packages/go_router/test/name_case_test.dart

+      assert(GoRouter.of(navKey.currentContext!).location == '/path');
+
+      // go to ScreenA
+      navKey.currentContext!.goNamed('Name');


Suggested change

navKey.currentContext!.goNamed('Name');

router.goNamed('Name');

chunhtai · 2023-06-12T22:16:20Z

packages/go_router/test/name_case_test.dart

+
+      // go to ScreenA
+      navKey.currentContext!.goNamed('Name');
+      assert(GoRouter.of(navKey.currentContext!).location == '/');


expect(find.byType(ScreenA), findsOneWidget);

Suggestions are applied, thanks ✌️

chunhtai · 2023-06-12T22:17:38Z

Looks like some other tests are failing

….byType() instead

Khalidm98 · 2023-06-13T00:49:14Z

Looks like some other tests are failing

@chunhtai Tests are fixed now

hannah-hyj

LGTM

Khalidm98 · 2023-06-13T17:03:44Z

@chunhtai , @Hangyujin
look like auto-submit failed with 401, can you give it a look? or am I missing something?

chunhtai · 2023-06-13T17:08:45Z

The ci was acting weird, I think the action item is on the flutter team to fix it.

Khalidm98 · 2023-06-13T17:13:45Z

Do they know about the issue? or do we need to raise a flag somehow?

Khalidm98 · 2023-06-13T18:40:29Z

It is released now, thank guys for your cooperation ✌️

…hing `_nameToPath` (flutter/packages#4196)

flutter/packages@0507297...f9314a3 2023-06-14 stuartmorgan@google.com [tool] Support running main.dart (flutter/packages#4208) 2023-06-13 stuartmorgan@google.com [tool] Allow running from anywhere (flutter/packages#4199) 2023-06-13 62033170+Khalidm98@users.noreply.github.com [go_router] #127016 preserved route name case when caching `_nameToPath` (flutter/packages#4196) 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 flutter-ecosystem@google.com,rmistry@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Khalidm98 added 2 commits June 12, 2023 16:44

#127016 preserved route name case when caching _nameToPath by not u…

7ab3bc6

…sing `toLowerCase()``

[go_router] #127016 updated version and changelog

5de2973

Khalidm98 requested a review from chunhtai as a code owner June 12, 2023 14:02

flutter-dashboard bot added the needs tests label Jun 12, 2023

github-actions bot added the p: go_router label Jun 12, 2023

chunhtai reviewed Jun 12, 2023

View reviewed changes

Khalidm98 and others added 4 commits June 13, 2023 00:42

[go_router] #127016 added name_case_test

a281789

Merge branch 'main' into main

0ad1569

[go_router] #127016 pulled main and resolved conflicts

e4e8e67

[go_router] #127016 added copyright and license text

0e2286d

chunhtai approved these changes Jun 12, 2023

View reviewed changes

Khalidm98 added 3 commits June 13, 2023 02:48

[go_router] #127016 removed navKey from test and used router and find…

992bd3c

….byType() instead

[go_router] #127016 fixed failed tests

fd9d8db

[go_router] #127016 fixed format

1116926

chunhtai requested a review from hannah-hyj June 13, 2023 15:30

hannah-hyj approved these changes Jun 13, 2023

View reviewed changes

hannah-hyj added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 13, 2023

auto-submit bot merged commit 1dad759 into flutter:main Jun 13, 2023

engine-flutter-autoroll mentioned this pull request Jun 14, 2023

Roll Packages from 050729760b25 to f9314a3b833a (3 revisions) flutter/flutter#128878

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 14, 2023

1dad759 [go_router] flutter#127016 preserved route name case when cac…

fcfa360

…hing `_nameToPath` (flutter/packages#4196)

This was referenced Jul 7, 2023

[Tool] New tool to download android dependencies #4408

Merged

[camera] tree fix format 2023 05 12 #4450

Closed

reidbaker mentioned this pull request Jul 14, 2023

[tooling] mark infra steps as infra steps in yaml files #4473

Merged

11 tasks

reidbaker mentioned this pull request Jul 26, 2023

[Tooling] Add google owned cache for dependencies as an option in ci #4567

Merged

11 tasks

Khalidm98 mentioned this pull request Aug 2, 2023

[go_router] Preserve route name case when caching _nameToPath flutter/flutter#127016

Closed

2 tasks

reidbaker mentioned this pull request Aug 4, 2023

[All] Expand artifact hub to all plugins #4645

Merged

11 tasks

		- Preserves route name case when caching `_nameToPath` by not using `toLowerCase()`.<br>
		this is useful to make route names case sensitive

	- Preserves route name case when caching `_nameToPath` by not using `toLowerCase()`.<br>
	this is useful to make route names case sensitive
	- Makes namedLocation and route name related APIs case sensitive.

	navKey.currentContext!.goNamed('name');
	router.goNamed('name');

	assert(GoRouter.of(navKey.currentContext!).location == '/path');
	expect(find.byType(ScreenB), findsOneWidget);

[go_router] #127016 preserved route name case when caching _nameToPath #4196

[go_router] #127016 preserved route name case when caching _nameToPath #4196

Uh oh!

Conversation

Khalidm98 commented Jun 12, 2023

Uh oh!

flutter-dashboard bot commented Jun 12, 2023

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!

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!

chunhtai commented Jun 12, 2023

Uh oh!

Khalidm98 commented Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hannah-hyj left a comment

Choose a reason for hiding this comment

Uh oh!

Khalidm98 commented Jun 13, 2023

Uh oh!

chunhtai commented Jun 13, 2023

Uh oh!

Khalidm98 commented Jun 13, 2023

Uh oh!

Khalidm98 commented Jun 13, 2023

Uh oh!

Uh oh!

[go_router] #127016 preserved route name case when caching `_nameToPath` #4196

[go_router] #127016 preserved route name case when caching `_nameToPath` #4196

Khalidm98 commented Jun 13, 2023 •

edited

Loading