Skip to content

[go_router_builder] Adds support for enhanced enums. - #2395

Merged
auto-submit[bot] merged 15 commits into
flutter:mainfrom
Skogsfrae:main
Sep 16, 2022
Merged

[go_router_builder] Adds support for enhanced enums.#2395
auto-submit[bot] merged 15 commits into
flutter:mainfrom
Skogsfrae:main

Conversation

@Skogsfrae

@Skogsfrae Skogsfrae commented Jul 28, 2022

Copy link
Copy Markdown
Contributor

Fixes #105876

Added support to enhanced enums introduced in dart 2.17

Pre-launch Checklist

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

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

@flutter-dashboard

Copy link
Copy Markdown

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.

@Skogsfrae Skogsfrae changed the title [WIP] fixes broken enhanced enums. Fixes broken enhanced enums. Jul 28, 2022
@Skogsfrae Skogsfrae changed the title Fixes broken enhanced enums. [go_router_builder] Fixes broken enhanced enums. Jul 28, 2022
Comment thread packages/go_router_builder/CHANGELOG.md Outdated
@loic-sharma

Copy link
Copy Markdown
Member

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.

@Skogsfrae

Copy link
Copy Markdown
Contributor Author

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;
}

@chunhtai chunhtai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you add the enum to the all_types.dart example so the test can on the example?

also would be good to write a test in example/test similar to widget_test.dart to verify the generated content can run without issue.

@Skogsfrae

Copy link
Copy Markdown
Contributor Author

@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?

@chunhtai

chunhtai commented Aug 9, 2022

Copy link
Copy Markdown
Contributor

@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?

@Skogsfrae

Skogsfrae commented Aug 11, 2022

Copy link
Copy Markdown
Contributor Author

@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?

@chunhtai

chunhtai commented Aug 25, 2022

Copy link
Copy Markdown
Contributor

@Skogsfrae

Yes that is a good idea, please go ahead.

@chunhtai

chunhtai commented Sep 8, 2022

Copy link
Copy Markdown
Contributor

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

@Skogsfrae

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor Author

@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.

@chunhtai chunhtai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

code LGTM, it looks like there are some formatting issue
https://github.com/flutter/packages/pull/2395/checks?check_run_id=8372456407

Comment thread packages/go_router_builder/CHANGELOG.md Outdated

@chunhtai chunhtai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do these need to be real links? This could cause security issues...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are these all named *FieldField now instead of just *Field?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My fault, I use a find and replace all 😬

Comment thread packages/go_router_builder/CHANGELOG.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Fixes broken code generation for enhanced enums. [#105876](https://github.com/flutter/flutter/issues/105876).
* Adds support for enhanced enums. [#105876](https://github.com/flutter/flutter/issues/105876).

@johnpryan johnpryan changed the title [go_router_builder] Fixes broken enhanced enums. [go_router_builder] Adds support for enhanced enums. Sep 15, 2022

@johnpryan johnpryan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM overall - my comments are mostly minor

@johnpryan

Copy link
Copy Markdown
Contributor

@Skogsfrae could you fix the merge conflicts?

@johnpryan johnpryan added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App needs tests p: go_router_builder

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[go_router_builder] Support Enhanced enums

4 participants