Skip to content

Clarified '/' route in initialRoute docs#64793

Closed
markusaksli-nc wants to merge 0 commit intoflutter:masterfrom
NevercodeHQ:initialroute_docs_fix
Closed

Clarified '/' route in initialRoute docs#64793
markusaksli-nc wants to merge 0 commit intoflutter:masterfrom
NevercodeHQ:initialroute_docs_fix

Conversation

@markusaksli-nc
Copy link
Contributor

@markusaksli-nc markusaksli-nc commented Aug 28, 2020 •
edited
Loading

Description

This PR clarifies the fact that Navigator will always load the / route as the root of initialRoute deep links and that even simple routes like /a will be treated as deep links (this would mean two routes are pushed). This is to deter new users from trying to use this property when they could just use home or routes: { '/': (context) =>... instead. The resulting confusion can be seen in the linked issue.

Related Issues

Fixes #64775

Tests

Not necessary, this is a documentation PR.

Checklist

Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

  • No, no existing tests failed, so this is not a breaking change.

@flutter-dashboard
Copy link

flutter-dashboard bot commented Aug 28, 2020

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.

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

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Aug 28, 2020
@markusaksli-nc
Copy link
Contributor Author

markusaksli-nc commented Aug 28, 2020

@chunhtai Not that familiar with the new Router API yet, maybe something can be added to mention that?

However, in practice it will be discouraged to provide an initialRoute string as well as an initial list of Pages. Instead, the initial route should just be defined as a list of Pages and the initialRoute string should be left as null.

From the doc

@markusaksli-nc markusaksli-nc changed the title initialRoute docs fix Clarified '/' route in initialRoute docs Aug 28, 2020
@sidrao2006
Copy link
Contributor

sidrao2006 commented Aug 28, 2020 •
edited
Loading

Could you please explain the reason why the home is better than initialRoute in the docs

@markusaksli-nc
Copy link
Contributor Author

markusaksli-nc commented Aug 28, 2020

@sidrao2006 I thought the reasoning was implicit enough. The reason was that if you want to use a named route scheme with names like /a, /b, /c then you should not try to hardcode one of these as initialRoute.

If you have defined / this would result in the Navigator pushing two routes (/ and /a for example) every time you launch the app. This is undesirable in most cases and confusing if you are not aware of this fact.

If you have not defined / then you want to push a certain page as the root of your app every time you launch. In this case, there is no need to use initialRoute, you can simply set that page as the root of your app with home. If you use initialRoute then the Navigator would attempt to push a meaningless deep link every time you launch and you would not have the benefit of having a defined default route in your app.

Only if you want to push a different root depending on the launch context would you leave / undefined and even then you would have a system in place to set initialRoute, you wouldn't just hardcode it. But in this case, you probably shouldn't be using the deep link naming scheme, to begin with.

Obviously these are not hard rules, once a user is familiar with routes they can do as they please with the system. This is simply to deter new users from getting confused when attempting to use initialRoute.

I fear this might be too verbose since this is just targeted at new users but I could add it if necessary.

@sidrao2006
Copy link
Contributor

sidrao2006 commented Aug 29, 2020

Ok thanks a lot. I just figured out that I was using initialRoute without any use for it.
It would be great if you could add it.

@chunhtai chunhtai self-requested a review August 31, 2020 16:25
@chunhtai
Copy link
Contributor

chunhtai commented Sep 2, 2020

Could you please explain the reason why the home is better than initialRoute in the docs

It is not better, it just less boiler plate when you don't care about routing. It essentially just push a route '/' to the navigator with its content = home.

chunhtai
chunhtai reviewed Sep 2, 2020
View reviewed changes
packages/flutter/lib/src/widgets/app.dart Outdated
///
/// Even if the route was just `/a`, the app would start with `/` and `/a`
/// loaded. This is why it is preferable to avoid hardcoding a deep link
/// [initialRoute] to simply set the default route of the app. The [home]
Copy link
Contributor

@chunhtai chunhtai Sep 2, 2020

Choose a reason for hiding this comment

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

I don't think we should encourage the usage of home for this reason. home is just less boilerplate code and may be confusing when uses with the WidgetsApp.routes

packages/flutter/lib/src/widgets/app.dart Outdated
/// also. For example, if the route was `/a/b/c`, then the app would start
/// with the four routes `/`, `/a`, `/a/b`, and `/a/b/c` loaded, in that order.
///
/// Even if the route was just `/a`, the app would start with `/` and `/a`
Copy link
Contributor

@chunhtai chunhtai Sep 2, 2020

Choose a reason for hiding this comment

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

would be good to mention that you can provide onGenerateInitialRoutes to override this behavior

@chunhtai
Copy link
Contributor

chunhtai commented Sep 2, 2020

@chunhtai Not that familiar with the new Router API yet, maybe something can be added to mention that?

However, in practice it will be discouraged to provide an initialRoute string as well as an initial list of Pages. Instead, the initial route should just be defined as a list of Pages and the initialRoute string should be left as null.

From the doc

pages api is not available in WidgetsApp, so you don't need to worry about it here

@Piinks Piinks added d: api docs Issues with https://api.flutter.dev/ documentation f: routes Navigator, Router, and related APIs. labels Sep 2, 2020
@markusaksli-nc
Copy link
Contributor Author

markusaksli-nc commented Sep 7, 2020

@chunhtai Yeah I agree, the suggestion to use home isn't that justified. I committed the changes.

chunhtai
chunhtai approved these changes Sep 8, 2020
View reviewed changes
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

@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Sep 8, 2020

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite testonly_devicelab_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

@chunhtai
Copy link
Contributor

chunhtai commented Sep 8, 2020

@markusaksli-nc Can you rebase off the master to see if it fixes the failing test?

@markusaksli-nc
Copy link
Contributor Author

markusaksli-nc commented Sep 9, 2020

Fork master was behind so the rebase got really messy, I'll just make a clean PR

@markusaksli-nc markusaksli-nc mentioned this pull request Sep 9, 2020
Merged
9 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@chunhtai chunhtai chunhtai approved these changes

@Piinks Piinks Awaiting requested review from Piinks

+1 more reviewer

@sidrao2006 sidrao2006 sidrao2006 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

d: api docs Issues with https://api.flutter.dev/ f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

MaterialApp onGenerateRoute show unexpected behaviour.

6 participants

@markusaksli-nc @sidrao2006 @chunhtai @fluttergithubbot @googlebot @Piinks