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

[Proposal] Router api allow to replace state #116491

Open
maRci002 opened this issue Dec 4, 2022 · 2 comments
Open

[Proposal] Router api allow to replace state #116491

maRci002 opened this issue Dec 4, 2022 · 2 comments
Labels
c: new feature Nothing broken; request for a new capability c: proposal A detailed proposal for a change to Flutter f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels. P3 Issues that are less important to the Flutter project team-framework Owned by Framework team triaged-framework Triaged by Framework team

Comments

@maRci002
Copy link
Contributor

maRci002 commented Dec 4, 2022

Use case

Imagine a situation where user wants to navigate to / then the app starts to check authentication status which is currently unknown (neither loggen in nor logged out) so I don't want to replace current url however I want to put some state in history (for instance to detect forward / backward actions).

In JS world History.replaceState's 3. parameter url is optional to achive this kind of behavior or just by passing '', null, undefined
as the third parameter.

Currently RouteInformationParser.restoreRouteInformation can return null to indicate not to update history.

This may return null, in which case the browser history will not be updated and state restoration is disabled.

RouteInformation's location property can also be null however using null will throw exception (what is the meaning of null?).

If I pass an empty string '' to RouteInformation.location then it will cause unexpected results like this:

  1. User opens app /
  2. RouteInformationParser.restoreRouteInformation returns RouteInformation(location: '', state: state) since authentication status is unknown
  3. browser replaces the state
  4. authentication status changed so RouteInformationParser.restoreRouteInformation returns RouteInformation(location: '/', state: state)
  5. browser history stack contains two '/' instead of one.

Proposal

If you look at PlatformRouteInformationProvider's current implementation:

  @override
  void routerReportsNewRouteInformation(RouteInformation routeInformation, {RouteInformationReportingType type = RouteInformationReportingType.none}) {
    final bool replace =
      type == RouteInformationReportingType.neglect ||
      (type == RouteInformationReportingType.none &&
       _valueInEngine.location == routeInformation.location);
    SystemNavigator.selectMultiEntryHistory();
    SystemNavigator.routeInformationUpdated(
      location: routeInformation.location!,
      state: routeInformation.state,
      replace: replace,
    );
    _value = routeInformation;
    _valueInEngine = routeInformation;
  }
  1. Documentation should clarify what null means in RouteInformation.location.
  2. _value / _valueInEngine shouldn't save RouteInformation if it's location is '' just copy the old one with the new state.
  3. final bool replace should handle specials cases like when RouteInformation.location is '' or null.

This is a possible implementation:

  @override
  void routerReportsNewRouteInformation(
    RouteInformation routeInformation, {
    RouteInformationReportingType type = RouteInformationReportingType.none,
  }) {
    final isEmptyLocation = routeInformation.location == null || routeInformation.location == '';

    final replace = type == RouteInformationReportingType.neglect ||
        (type == RouteInformationReportingType.none && (isEmptyLocation || _valueInEngine.location == routeInformation.location));
    SystemNavigator.selectMultiEntryHistory();
    SystemNavigator.routeInformationUpdated(
      // in case of [isEmptyLocation] we can also use old location [_valueInEngine.location!]
      location: isEmptyLocation ? '' : routeInformation.location!,
      state: routeInformation.state,
      replace: replace,
    );

    // TODO: _valueInEngine.copyWith(state: routeInformation.state) would be much nicer
    final newRouteInformation = isEmptyLocation
        ? RouteInformation(
            location: _valueInEngine.location,
            state: routeInformation.state,
          )
        : routeInformation;

    _value = newRouteInformation;
    _valueInEngine = newRouteInformation;
  }
@exaby73 exaby73 added the in triage Presently being triaged by the triage team label Dec 5, 2022
@exaby73
Copy link
Member

exaby73 commented Dec 5, 2022

Hello @maRci002. Thank you for raising this proposal. Labeling for further insights from the team

@exaby73 exaby73 added c: new feature Nothing broken; request for a new capability framework flutter/packages/flutter repository. See also f: labels. f: routes Navigator, Router, and related APIs. c: proposal A detailed proposal for a change to Flutter and removed in triage Presently being triaged by the triage team labels Dec 5, 2022
@goderbauer
Copy link
Member

cc @chunhtai

@goderbauer goderbauer added the P3 Issues that are less important to the Flutter project label Dec 6, 2022
@flutter-triage-bot flutter-triage-bot bot added team-framework Owned by Framework team triaged-framework Triaged by Framework team labels Jul 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: new feature Nothing broken; request for a new capability c: proposal A detailed proposal for a change to Flutter f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels. P3 Issues that are less important to the Flutter project team-framework Owned by Framework team triaged-framework Triaged by Framework team
Projects
None yet
Development

No branches or pull requests

3 participants