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

Allow gaps in the initial route #39440

Merged
merged 3 commits into from Sep 5, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 6 additions & 4 deletions packages/flutter/lib/src/widgets/app.dart
Expand Up @@ -361,10 +361,12 @@ class WidgetsApp extends StatefulWidget {
/// also. For example, if the route was `/a/b/c`, then the app would start
/// with the three routes `/a`, `/a/b`, and `/a/b/c` loaded, in that order.
///
/// If any part of this process fails to generate routes, then the
/// [initialRoute] is ignored and [Navigator.defaultRouteName] is used instead
/// (`/`). This can happen if the app is started with an intent that specifies
/// a non-existent route.
/// Intermediate routes aren't required to exist. In the example above, `/a`
/// and `/a/b` could be skipped if they have no matching route. But `/a/b/c` is
/// required to have a route, else [initialRoute] is ignored and
/// [Navigator.defaultRouteName] is used instead (`/`). This can happen if the
/// app is started with an intent that specifies a non-existent route.
///
/// The [Navigator] is only built if routes are provided (either via [home],
/// [routes], [onGenerateRoute], or [onUnknownRoute]); if they are not,
/// [initialRoute] must be null and [builder] must not be null.
Expand Down
23 changes: 12 additions & 11 deletions packages/flutter/lib/src/widgets/navigator.dart
Expand Up @@ -783,6 +783,15 @@ class Navigator extends StatefulWidget {
/// then the [Navigator] would push the following routes on startup: `/`,
/// `/stocks`, `/stocks/HOOLI`. This enables deep linking while allowing the
/// application to maintain a predictable route history.
///
/// If any of the intermediate routes doesn't exist, it'll simply be skipped.
/// In the example above, if `/stocks` doesn't have a corresponding route in
/// the app, it'll be skipped and only `/` and `/stocks/HOOLI` will be pushed.
///
/// That said, the full route has to map to something in the app in order for
/// this to work. In our example, `/stocks/HOOLI` has to map to a route in the
/// app. Otherwise, [initialRoute] will be ignored and [defaultRouteName] will
/// be used instead.
final String initialRoute;

/// Called to generate a route for a given [RouteSettings].
Expand Down Expand Up @@ -1509,9 +1518,6 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin {
if (initialRouteName.startsWith('/') && initialRouteName.length > 1) {
initialRouteName = initialRouteName.substring(1); // strip leading '/'
assert(Navigator.defaultRouteName == '/');
final List<String> plannedInitialRouteNames = <String>[
Navigator.defaultRouteName,
];
final List<Route<dynamic>> plannedInitialRoutes = <Route<dynamic>>[
_routeNamed<dynamic>(Navigator.defaultRouteName, allowNull: true, arguments: null),
];
Expand All @@ -1520,30 +1526,25 @@ class NavigatorState extends State<Navigator> with TickerProviderStateMixin {
String routeName = '';
for (String part in routeParts) {
routeName += '/$part';
plannedInitialRouteNames.add(routeName);
plannedInitialRoutes.add(_routeNamed<dynamic>(routeName, allowNull: true, arguments: null));
}
}
if (plannedInitialRoutes.contains(null)) {
if (plannedInitialRoutes.last == null) {
assert(() {
FlutterError.reportError(
FlutterErrorDetails(
Copy link
Member

Choose a reason for hiding this comment

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

This exception message probably needs updating to reflect the new behavior?

Copy link
Member

Choose a reason for hiding this comment

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

The documentation on various initialRoute arguments in Flutter (e.g. on Navigator, MaterialApp, etc) needs to be updated to reflect the new behavior.

exception:
'Could not navigate to initial route.\n'
'The requested route name was: "/$initialRouteName"\n'
'The following routes were therefore attempted:\n'
' * ${plannedInitialRouteNames.join("\n * ")}\n'
Copy link
Member

Choose a reason for hiding this comment

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

Seems like the variable plannedInitialRouteNames could be eliminated altogether then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still use it below to push the routes.

Copy link
Member

Choose a reason for hiding this comment

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

What line? Doesn't the pushing use plannedInitialRoutes, not plannedInitialRouteNames?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I misread your previous comment, thought you were talking about plannedInitialRoutes.

Yes, the variable plannedInitialRouteNames can be eliminated. Good catch!

'This resulted in the following objects:\n'
' * ${plannedInitialRoutes.join("\n * ")}\n'
'One or more of those objects was null, and therefore the initial route specified will be '
'There was no corresponding route in the app, and therefore the initial route specified will be '
'ignored and "${Navigator.defaultRouteName}" will be used instead.'
),
);
return true;
}());
push(_routeNamed<Object>(Navigator.defaultRouteName, arguments: null));
} else {
plannedInitialRoutes.forEach(push);
plannedInitialRoutes.where((Route<dynamic> route) => route != null).forEach(push);
}
} else {
Route<Object> route;
Expand Down
60 changes: 60 additions & 0 deletions packages/flutter/test/widgets/navigator_test.dart
Expand Up @@ -973,4 +973,64 @@ void main() {
expect(arguments.single, 'pushReplacementNamed');
arguments.clear();
});

testWidgets('Initial route can have gaps', (WidgetTester tester) async {
final GlobalKey<NavigatorState> keyNav = GlobalKey<NavigatorState>();
const Key keyRoot = Key('Root');
const Key keyA = Key('A');
const Key keyABC = Key('ABC');

await tester.pumpWidget(
MaterialApp(
navigatorKey: keyNav,
initialRoute: '/A/B/C',
routes: <String, WidgetBuilder>{
'/': (BuildContext context) => Container(key: keyRoot),
'/A': (BuildContext context) => Container(key: keyA),
// The route /A/B is intentionally left out.
'/A/B/C': (BuildContext context) => Container(key: keyABC),
},
),
);

// The initial route /A/B/C should've been pushed successfully.
expect(find.byKey(keyRoot), findsOneWidget);
expect(find.byKey(keyA), findsOneWidget);
expect(find.byKey(keyABC), findsOneWidget);

keyNav.currentState.pop();
await tester.pumpAndSettle();
expect(find.byKey(keyRoot), findsOneWidget);
expect(find.byKey(keyA), findsOneWidget);
expect(find.byKey(keyABC), findsNothing);
});

testWidgets('The full initial route has to be matched', (WidgetTester tester) async {
final GlobalKey<NavigatorState> keyNav = GlobalKey<NavigatorState>();
const Key keyRoot = Key('Root');
const Key keyA = Key('A');
const Key keyAB = Key('AB');

await tester.pumpWidget(
MaterialApp(
navigatorKey: keyNav,
initialRoute: '/A/B/C',
routes: <String, WidgetBuilder>{
'/': (BuildContext context) => Container(key: keyRoot),
'/A': (BuildContext context) => Container(key: keyA),
'/A/B': (BuildContext context) => Container(key: keyAB),
// The route /A/B/C is intentionally left out.
},
),
);

final dynamic exception = tester.takeException();
expect(exception is String, isTrue);
expect(exception.startsWith('Could not navigate to initial route.'), isTrue);

// Only the root route should've been pushed.
expect(find.byKey(keyRoot), findsOneWidget);
expect(find.byKey(keyA), findsNothing);
expect(find.byKey(keyAB), findsNothing);
});
}