packages icon indicating copy to clipboard operation
packages copied to clipboard

[go_router_builder] Fixes broken enhanced enums.

Open Skogsfrae opened this issue 3 years ago • 6 comments

Fixes #105876

Added support to enhanced enums introduced in dart 2.17

Pre-launch Checklist

  • [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
  • [x] I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • [x] I signed the CLA.
  • [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • [x] I listed at least one issue that this PR fixes in the description above.
  • [x] I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • [x] I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • [ ] I updated/added relevant documentation (doc comments with ///).
  • [x] I added new tests to check the change I am making, or this PR is test-exempt.
  • [x] All existing and new tests are passing.

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

Skogsfrae avatar Jul 28 '22 18:07 Skogsfrae

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.

flutter-dashboard[bot] avatar Jul 28 '22 18:07 flutter-dashboard[bot]

Thanks for sending this pull request. Could you explain what scenarios you'd want to use an enhanced enum instead of a regular enum for your routes? It's not clear to me that we should allow enhanced enums since we can't map all of their fields cleanly to parameters. In my mind it would be better to produce an error if an enhanced enum is used that recommends using a regular enum instead.

loic-sharma avatar Jul 29 '22 17:07 loic-sharma

Thanks for sending this pull request. Could you explain what scenarios you'd want to use an enhanced enum instead of a regular enum for your routes? It's not clear to me that we should allow enhanced enums since we can't map all of their fields cleanly to parameters. In my mind it would be better to produce an error if an enhanced enum is used that recommends using a regular enum instead.

In my production use case I switched from adding functionalities to an enum with extensions to enhanced enums that allow to write less code and achieve the same result with a more concise and straight forward way, especially for json serialization/deserialization.

Mapping an enhanced enum fields should not be done since according to dart specification they must be final, unmutable and can only be initialised by each enum value calling a const constructor.

Dart documentation

To declare an enhanced enum, follow a syntax similar to normal classes, but with a few extra requirements:

Instance variables must be final, including those added by mixins. All generative constructors must be constant. Factory constructors can only return one of the fixed, known enum instances. No other class can be extended as Enum is automatically extended. There cannot be overrides for index, hashCode, the equality operator ==. A member named values cannot be declared in an enum, as it would conflict with the automatically generated static values getter. All instances of the enum must be declared in the beginning of the declaration, and there must be at least one instance declared.

An example of use is pairing each enum value with additional useful data:

Enum definition
enum SportEnum {
  volleyball(
    imageUrl: 'https://i.cbc.ca/1.6477752.1654316355!/fileImage/httpImage/image.jpg_gen/derivatives/16x9_940/canada-vs-usa-volleyball.jpg',
    playerPerTeam: 6,
    accessory: null,
    hasNet: true,
  ),
  football(
    imageUrl: 'https://upload.wikimedia.org/wikipedia/commons/b/b9/Football_iu_1996.jpg',
    playerPerTeam: 11,
    accessory: null,
    hasNet: true,
  ),
  tennis(
    imageUrl: 'https://upload.wikimedia.org/wikipedia/commons/9/94/2013_Australian_Open_-_Guillaume_Rufin.jpg',
    playerPerTeam: 2,
    accessory: 'Rackets',
    hasNet: true,
  ),
  hockey(
    imageUrl: 'https://upload.wikimedia.org/wikipedia/commons/3/39/Pittsburgh_Penguins%2C_Washington_Capitals%2C_Bryan_Rust_%2833744033514%29.jpg',
    playerPerTeam: 6,
    accessory: 'Hockey sticks',
    hasNet: true,
  ),
  ;

  final String imageUrl;
  final int playerPerTeam;
  final String? accessory;
  final bool hasNet;

  const SportEnum({
    required this.accessory,
    required this.hasNet,
    required this.imageUrl,
    required this.playerPerTeam,
  });
}
Route declaration
@TypedGoRoute<SportDetailPageData>(path: '/sport/:sport')
class SportDetailPageData extends GoRouteData {
  SportDetailPageData(this.sport);
  final SportEnum sport;

  @override
  Widget build(BuildContext) => SportDetailPage(
    name: sport.name,
    image: sport.imageUrl,
    playerPerTeam: sport.playerPerTeam,
    hasNet: sport.hasNet,
    accessory: sport.accessory,
  );
}
Generated code

GoRoute get $sportDetailPageData => GoRouteData.$route(
      path: '/sport/:sport',
      factory: $SportDetailPageDataExtension._fromState,
    );

extension $SportDetailPageDataExtension on SportDetailPageData {
  static SportDetailPageData _fromState(GoRouterState state) =>
      SportDetailPageData(
        _$SportEnumEnumMap._$fromName(state.params['sport']!),
      );

  String get location => GoRouteData.$location(
        '/sport/${Uri.encodeComponent(_$SportEnumEnumMap[sport]!)}',
      );

  void go(BuildContext context) => context.go(location, extra: this);

  void push(BuildContext context) => context.push(location, extra: this);
}

const _$SportEnumEnumMap = {
  SportEnum.volleyball: 'volleyball',
  SportEnum.football: 'football',
  SportEnum.tennis: 'tennis',
  SportEnum.hockey: 'hockey',
};

extension<T extends Enum> on Map<T, String> {
  T _$fromName(String value) =>
      entries.singleWhere((element) => element.value == value).key;
}

Skogsfrae avatar Jul 30 '22 17:07 Skogsfrae

@chunhtai i add an enhanced enum field to all_types.dart example as requested and I also fixed a bug in it (routeInformationProvider was missing and initial route '/' wouldn't be found resulting in an error page).

Regarding widget_test.dart is a test against the app written in main.dart with a complete navigation tree, while on the contrary all_types.dart has only a single path with all the possible data combination.

Should I put an example in main.dart and then its test in widget_test.dart or a complete separate example app with its test suite?

Skogsfrae avatar Aug 09 '22 08:08 Skogsfrae

@chunhtai i add an enhanced enum field to all_types.dart example as requested and I also fixed a bug in it (routeInformationProvider was missing and initial route '/' wouldn't be found resulting in an error page).

Regarding widget_test.dart is a test against the app written in main.dart with a complete navigation tree, while on the contrary all_types.dart has only a single path with all the possible data combination.

Should I put an example in main.dart and then its test in widget_test.dart or a complete separate example app with its test suite?

Is it possible to modify the all_types to add a new route so it can navigate in between?

chunhtai avatar Aug 09 '22 15:08 chunhtai

@chunhtai instead of just running a single route with all types I could refactor the all_types app to make it more structured with a dedicated route per type and run a complete test against the entire app. Would it be ok?

Skogsfrae avatar Aug 11 '22 16:08 Skogsfrae

@Skogsfrae

Yes that is a good idea, please go ahead.

chunhtai avatar Aug 25 '22 21:08 chunhtai

(triage) Hi @Skogsfrae, are you still planning to come back to this pr?

chunhtai avatar Sep 08 '22 18:09 chunhtai

Hi @chunhtai Yes, I've been on holiday and came back on track in these days 💪🏽 I've already rewritten the all types app and tomorrow I'll write the test suite, then it'll be ready to go

Skogsfrae avatar Sep 08 '22 18:09 Skogsfrae

@chunhtai I rebased my branch to upstream/main and updated release info. You can review the change in example/lib/all_types.dart and the tests in example/test/all_types_test.dart

Let me know if more changes need to be done.

Skogsfrae avatar Sep 15 '22 12:09 Skogsfrae

@Skogsfrae could you fix the merge conflicts?

johnpryan avatar Sep 16 '22 00:09 johnpryan