Package Proposal: redux_undo

[michelengelen]

Package Proposal: redux_undo

Dependency name (as used in pubspec.yaml): redux_undo Current pub.dev link: https://pub.dev/packages/redux_undo Current Git repository link: https://github.com/michelengelen/redux_undo Description: redux_undo gives you the possibility to go back in the state history making undo and redo actions possible Current maintainer: Michel Engelen, michel@nomorejs.net, michelengelen Needs new maintainer after transfer: NO (could be better with some more contributors, though) New maintainer (if applicable): / Reason for transfer: I think it is a good use-case to have the possibilities it provides in a redux-based application. Maybe there are some developers that want to help improving this package Comments: It is currently working as is, but could maybe be improved with more help

[jeroen-meijer]

Hi there,

Thanks for submitting a package request. This looks pretty nice.

I need time to review this thoroughly to see how it works in actual Redux applications.

Stay tuned for more info soon.

Best regards. 👋🏻

[brianegan]

Hey, I think this package looks really cool, overall :) Thanks for sharing!

After looking at the code, I think it might make sense to make a few improvements before the community considers taking it over:

1. UndoableState should be typed UndoableState<S>, where S is a generic type parameter of the State class, e.g. UndoableState<AppState>. The lists of previous / next / etc states should be typed List<S>, not List<dynamic>. With dynamic, folks lose all type information about their state objects when working with UndoableState.
2. Example app does not run and must be updated for latest version of Redux + flutter_redux
3. Fix 20 analysis errors
4. Consider formatting with dartfmt
5. Add a suite of tests that exercise the functionality of your package to make sure everything works as expected. Very important for long-term maintenance and a signal of quality.
6. Once tests are in place, I'd also recommend adding a CI step to verify the tests pass when new PRs are submitted and merged to master

[jeroen-meijer]

Hi, all.

Thanks, @brianegan, for your feedback!

@michelengelen, please take a look at the feedback above and make the necessary changes. Once this is done, I'll perform a final review myself and we'll take it from there.

If you have any questions, please let me know!

Best regards 👋🏻

[michelengelen]

Hi @brianegan and @jeroen-meijer,

First of all: Thanks for the feedback ... Glad you liked it and very appreciated. ✨

I fear I need to ask you for help on the first point (I can do the others myself). I come from the JavaScript world and types are kinda new to me. I get most of it, but I have never encountered generic types before and when I make the changes you suggested my IDE yells at me! 😅

class UndoableState<S> works just fine as well as final List<S> past, final List<S> future and final s present. Nothing to worry here.

Problem is, that when I try to add it here: Reducer<UndoableState<S>> createUndoableReducer(S reducer, {UndoableConfig config})

It is not working (obviously) because there is no info on the type S here.

What would be the best way to fix that?

[brianegan]

No problem! It takes some time to get used to it coming from a dynamic language like JS for sure, but hopefully you'll start to feel the benefits over time!

The way to add type information to a function is like so:

Reducer<UndoableState<S>> createUndoableReducer<S>(Reducer<S> reducer,
    {UndoableConfig config}) {

or

UndoableState<S> newHistory<S>(List<S> past, S present, List<S> future) {

Basically, just after the name of the function and before the arguments, you'll put <S>, which let's the function define the generic type params. Does that make sense?

[michelengelen]

@brianegan absolutely ... I was just crawling the docs and found it out myself ... I have a lot of refactoring to do now! ;D

BTW: where did you find that 20 analysis errors?

[brianegan]

Haha, yep!

If you're using VScode or IntelliJ/Android Studio, there should be a window that shows all Dart errors. If you can't find it, then running flutter analyze from the root of the project should print out all analysis warnings / errors in your terminal.

[michelengelen]

I do use Android Studio and there are no analyse issues to be found ... I did run flutter analyze as well, but there are no errors found.

I did change the analysis_options.yaml in my new commit, but still ... no errors.

Current state:

• [x] generic classes
• [x] Example fix
• [x] analyzer errors
• [x] refactoring/formatting
• [x] tests
• [x] CI

[brianegan]

This is what I'm seeing on my side using the master branch of your library and stable version of Fltter, might be different if you've modified the analysis options though:

$ flutter analyze  
Running "flutter pub get" in redux_undo...                          0.6s
Analyzing redux_undo...                                                 

   info • Omit type annotations for local variables • lib/src/reducer.dart:17:5 • omit_local_variable_types
   info • Omit type annotations for local variables • lib/src/reducer.dart:24:5 • omit_local_variable_types
   info • Omit type annotations for local variables • lib/src/reducer.dart:32:7 • omit_local_variable_types
   info • Omit type annotations for local variables • lib/src/reducer.dart:35:7 • omit_local_variable_types
   info • Omit type annotations for local variables • lib/src/reducer.dart:38:7 • omit_local_variable_types
   info • Omit type annotations for local variables • lib/src/reducer.dart:41:7 • omit_local_variable_types
   info • Omit type annotations for local variables • lib/src/reducer.dart:45:5 • omit_local_variable_types
   info • Omit type annotations for local variables • lib/src/utils.dart:17:3 • omit_local_variable_types
   info • Omit type annotations for local variables • lib/src/utils.dart:45:3 • omit_local_variable_types
   info • Omit type annotations for local variables • lib/src/utils.dart:46:3 • omit_local_variable_types
   info • Omit type annotations for local variables • lib/src/utils.dart:49:3 • omit_local_variable_types
   info • Omit type annotations for local variables • lib/src/utils.dart:51:3 • omit_local_variable_types
   info • Omit type annotations for local variables • lib/src/utils.dart:62:3 • omit_local_variable_types
   info • Omit type annotations for local variables • lib/src/utils.dart:63:3 • omit_local_variable_types
   info • Omit type annotations for local variables • lib/src/utils.dart:66:3 • omit_local_variable_types
   info • Omit type annotations for local variables • lib/src/utils.dart:70:3 • omit_local_variable_types
   info • Omit type annotations for local variables • lib/src/utils.dart:86:3 • omit_local_variable_types
   info • Omit type annotations for local variables • lib/src/utils.dart:89:3 • omit_local_variable_types
   info • Omit type annotations for local variables • lib/src/utils.dart:91:3 • omit_local_variable_types
   info • Omit type annotations for local variables • lib/src/utils.dart:92:3 • omit_local_variable_types

20 issues found. (ran in 2.4s)

[michelengelen]

This is what I am getting:

$ flutter analyze
Analyzing redux_undo...                                                 
No issues found! (ran in 2.0s)

Strange!

I do not even have this linter rule activated: omit_local_variable_types

Could it be you have it globally activated or so?

[brianegan]

That rule comes from the pedantic package which is enabled by this line in the analysis options:

include: package:pedantic/analysis_options.yaml

Strange, indeed, though...

[michelengelen]

yeah, now I know where this comes from:

I do have pedantic in my dev-dependencies, but only with version ^1.8.0 which loads 1.8.0 for me (because the version in lock is sufficient for that version definition) ... I changed it to ^1.9.0 and now it is showing me those errors as well.

[jeroen-meijer]

Great work and communication, both of you 😁

Just a quick heads up @michelengelen; Once the requested from @brianegan are done, I'll review your package myself. One of the things I'll check is documentation, and I use the following linter rules:

linter:
  rules:
    ...
    - public_member_api_docs
    - package_api_docs
    - slash_for_doc_comments

So if you want, you can be one step ahead of me and already include those rules and make sure they're enforced so the review process will go faster 👍

[michelengelen]

@jeroen-meijer already have that sorted out ... I always use those rules as well ... but removed them for testing that "strange" behaviour we had before.

I will put them back in, so that there will be nothing to worry about! ✨✨✨✨

[michelengelen]

So I've added the first tests @brianegan.

What CI-tool would you prefer to test those? Would you rather go with Travis or simply use the built in github CI (workflows)?

Same question for codecoverage.

Thanks in advance!

[brianegan]

Hiya -- I actually haven't tried the new github actions just yet, but I think it'd be good to try! If you just wanna see a config that works with Travis + Codecov (service used for code coverage) , you could almost copy / paste this travis.yaml I wrote for the original redux project into redux_undo and make a couple adjustments.

This script:

1. Checks for analysis issues
2. Checks Dartfmt
3. Runs the tests and collects coverage
4. Uploads the coverage report to CodeCov

language: dart
sudo: false
dart:
- stable
- dev
with_content_shell: false
script:
  - dartanalyzer --fatal-infos --fatal-warnings ./
  - dartfmt -n ./lib --set-exit-if-changed
  - pub global activate coverage
  - pub run test test/redux_test.dart
  - dart --disable-service-auth-codes --enable-vm-service=8111 --pause-isolates-on-exit test/redux_test.dart &
  - nohup pub global run coverage:collect_coverage --port=8111 --out=coverage.json --wait-paused --resume-isolates
  - pub global run coverage:format_coverage --lcov --in=coverage.json --out=lcov.info --packages=.packages --report-on=lib
after_success:
  - bash <(curl -s https://codecov.io/bash)

[michelengelen]

Hey @brianegan and @jeroen-meijer

I have just now added all travis configs and a lot of tests (i think it does not cover 100% of the repo, but that will come!)

So, I think it is ready for another round of review! :D

[michelengelen]

@jeroen-meijer I have reached 100% coverage ... Ready to review! 🍡

[jeroen-meijer]

Great job! I’ll review it soon and will let you know once I’m done. @brianegan Think you can do a final check as well, to see if your changes were implemented properly? 😊

[jeroen-meijer]

Hello again @michelengelen

I've taken a look at the code again, and first off, great work on the changes! Just a couple more things and then it's all good on my end.

To do before approval

As soon as these issues are fixed, the package will get rereviewed.

1. UndoableConfig documentation. The UndoableConfig class has documentation, but it seems a bit vague. Please clarify and/or give examples for the limit, blackList and whiteList.
2. blackList and whiteList generic types. I see you've added generic types to the blackList and whiteList fields in UndoableConfig, which is great. 😁 However, you're using List<Type> for the type of both fields, which actually doesn't help you that much since you can provide any type in that list. For example:

   final UndoableConfig config = UndoableConfig(
   whiteList: <Type>[
   // Actual action.
   CustomUndo,
   // Non-related types should cause an error, but they don't.
   int,
   bool,
   String,
   Object,
   ],
   );

   int, bool, etc. are all types, so they're accepted values, but I don't think that's what you want. To my knowledge, there is currently no way to make this typesafe. The best thing to do here would be to add an assert in the UndoableConfig constructor that checks if every item in the blackList and whiteList is of type UndoableAction. That means that you need to edit all relevant action classes in actions.dart like so:

// NEW:
/// Base class for all actions.
abstract class UndoableAction {}

// EDITED:
/// Standard-Action for initiating the history
class UndoableInitAction extends UndoableAction {}

// EDITED:
/// Standard-Action for undo
class UndoableUndoAction extends UndoableAction {}

...etc...

If you want more help with this, please let me know. 🙂

3. Small optimizations Since I believe you're new to Dart, there's some stuff you might be unaware of but is nice to know. For example, you don't need to call .toString() if you're doing string interpolation. In classes.dart on line 66 for example:

   // This
   'UndoableState: List<$S> past: ${past.toString()}, ...'
   // is the same as this. .toString() gets called for you.
   'UndoableState: List<$S> past: $past, ...'

   Also, you can break up a string into multiple strings/lines for readability. For example, in the same file:

   
   // This
   @override
   String toString() =>
     'UndoableState: List<$S> past: ${past.toString()}, $S present: ${present.toString()}, List<$S> future: ${future.toString()}, $S latestUnfiltered: ${latestUnfiltered.toString()}, int index: $index';

// is the same as this. Don't forget the spaces at all but the last lines. @override String toString() => 'UndoableState: ' 'List<$S> past: ${past.toString()}, ' '$S present: ${present.toString()}, ' 'List<$S> future: ${future.toString()}, ' '$S latestUnfiltered: ${latestUnfiltered.toString()}, ' 'int index: $index';

Please check your codebase and make the above edits (shouldn't be that many).

## Changes for the future
These are changes that aren't required now but would be nice to have in the future, either by you or another community member.
1. **Prettier documentation.**
The code samples and explanations provided in the documentation are great. A spelling- and grammar-check would make it more readable (such as using proper casing and punctuation marks).

---

Hope all my feedback is clear. If you have any questions or need help, please feel free to let me know!

Best regards 👋🏻

[michelengelen]

@jeroen-meijer your are right ... I am into Flutter/Dart for the last 5-6 months now ... and loving it ❤️

I have made all the adjustments you mentioned above, except for the generic type on the Lists. Still trying to figure something out for that.

[jeroen-meijer]

Got it. I've taken a look at the changes, and they are great.

There's some stuff I'd like to see changed, but let's first get this baby in the community, and then I'll do a PR myself 😁

As for that last List thing, I would add an assert like so to UndoableConfig:

  UndoableConfig({
    this.limit = 10,
    this.blackList = const <Type>[],
    this.whiteList = const <Type>[],
  })  : assert(
          blackList.every((type) => type is UndoableAction),
          'All blackList types must extend UndoableAction.',
        ),
        assert(
          whiteList.every((type) => type is UndoableAction),
          'All whiteList types must extend UndoableAction.',
        );

Good luck!

[michelengelen]

Hi @jeroen-meijer ... the solution you provided does not work as it is stated, since you cannot check a type with is like you mentioned here. It will just throw an assertion error.

I will leave it as is and try to find another solution for the underlying problem ... maybe the way I am checking for black-/whitelisted types does not quite work in this case.

Let me think of something better.

Maybe something like this might work better:

abstract class BlacklistAction {}

class MyBlacklistedAction extends BlacklistAction { ... }

and then inside the reducer do something like this:

...
if (action is BlacklistAction) {
  ...
}
...

What do you think?

[jeroen-meijer]

Oh, I see. Good catch. Too bad that doesn't work.

Let's look at this issue at a later date. Maybe somebody else has a nice solution, or we'll have to resort to .runtimeType.

I wouldn't add the BlacklistAction. It makes it more difficult for users, and ideally, we'd want to catch incorrect as early as possible (i.e. during the class initialization).

---

Besides that, is there anything still left to do? If not, then this package is approved.

[michelengelen]

I dont think there is anything else to do at this stage ... I will add a new version and push it to pub when it is ok for now

[michelengelen]

So I have updated README, CHANGELOG, version and published it again ... Any objections so far? @brianegan or @jeroen-meijer?

[jeroen-meijer]

LGTM

Approved

Thanks for submitting this package. It has been reviewed and approved for release on the Flutter Community GitHub! 🎉 Great work. 👍🏻

What to do next

Please follow steps 3 through 7 in the Flutter Community Transfer Guide. Once you have done that, notify the package reviewer by posting a comment in this issue thread.

Good luck!

[michelengelen]

hey @jeroen-meijer ... all transfer steps are done. (y)

[jeroen-meijer]

Hey @michelengelen, great work.

I checked out your changes and had these small feedback points.

1. I noticed that the README contains the Flutter Community banner but you haven't replaced the PACKAGE_NAME in both places with redux_undo yet.
2. Are you sure that Flutter Community has been added as an uploader?

[jeroen-meijer]

I see the above changes have been submitted. You have been invited to be a part of the Flutter Community organization. Please check your email and accept the invite, then let me know here.

[michelengelen]

Hi @jeroen-meijer ... ping!

[jeroen-meijer]

Nice! You should now be able to follow step 8 of the transfer guide to transfer over your package to the community. After that, we should be all done.

[michelengelen]

@jeroen-meijer: Transfer is done ... I am looking forward to the future colaboration! ✨

[jeroen-meijer]

Awesome! Likewise :)