Nullability support

Question

Nullability support

AlexBacich opened this issue 4 years ago · 27 comments

Hey, do you have in plans nullability support for the library? I can take care about nullability support for Rx_widgets.

Answer 1 · 2020-12-05T21:54:50.000Z

yep will come, I only have to have time :-)
get_it and functional_listener are already migrated

Answer 2 · 2021-03-08T05:53:33.000Z

Morning everyone,

is this coming anytime soon? This library is currently the only one that is left to update our production code.
I've had a go at it myself, but as there are a number of ways to migrate the package, I don't really want to interfere with the intentions of the maintainers...

Thanks for your support!

Answer 3 · 2021-03-08T07:52:58.000Z

@mk-dev-1 , do you have migrated version? I've run into issues while trying to migrate.
Problematic piece - inputStream.materialize().

@escamoteur , hopefully you 'll have time to look into this. As been said, I'll take care of rx_widgets.

Answer 4 · 2021-03-08T09:15:16.000Z

@josh-ksr have you already done something on this side?

Answer 5 · 2021-03-08T09:16:20.000Z

@mk-dev-1 could you make a PR, so that we could have a look at it?

Answer 6 · 2021-03-08T19:30:42.000Z

@escamoteur: Please have a look at #49. I am struggling with an issue, but I am sure it can be resolved rather easily ;-)

Answer 7 · 2021-04-07T16:59:02.000Z

@escamoteur , would you have time to work on it any time soon? This seems too challenging to me to migrate myself, and this issue is blocking for full project migration to null-safety.

Answer 8 · 2021-04-08T06:57:03.000Z

Will try to do it over the weekend

Answer 9 · 2021-04-12T13:56:55.000Z

ok the weekend wasn't long enough. I'm thinking of making the API exactly (almost) the same as that of flutter_command. This would be a breaking change, though nothing that isn't quickly fixable. What are your thoughts?

Answer 10 · 2021-04-12T14:16:27.000Z

The whole null-safety is breaking change, so I think now it's the perfect time to introduce it. All logic will be re-tested anyway.

Answer 11 · 2021-04-21T16:21:45.000Z

just pushed first version 6.0.0-null-safety

https://github.com/fluttercommunity/rx_command/tree/null-safety

please have a close look and tell me what you think!

Answer 12 · 2021-04-21T18:21:07.000Z

Thank you so much for your work, much appreciated!!

I can see that initialLastResult has now become a required field as it is non-nullable. Previously I have not been needing any initial results, therefore I now have to add this for every RxCommand that I set up, which seems a bit verbose. From what I can see it is safe to put in a dummy instance of TResult as long as I don't use emitInitialCommandResult, right?

In any case, maybe there is a way to avoid making initialLastResult mandatory?

Answer 13 · 2021-04-22T07:50:27.000Z

Totally right. This came because for flutter_command it's mandatory as every ValueNotifier needs to have an initial value
Fixed with
6.0.0-null-safety.1

What would be absolutely awesome if someone could update the readme to the changed API, namely:

thrownExceptions now emits CommandErrors
CommandResult now contains the param value
renaming of canExecute to constraint
also check all source snippets
and if I forgot something else :-)

That would be a great help

Answer 14 · 2021-04-23T10:17:49.000Z

Here's another observation / question:

  static RxCommand<TParam, void> createAsyncNoResult<TParam>(
      AsyncAction1<TParam> action,
      {Stream<bool>? restriction,
      bool emitInitialCommandResult = false,
      bool emitsLastValueToNewSubscriptions = false,
      String? debugName}) {

where AsyncAction1 is defined as

typedef AsyncAction1<TParam> = Future Function(TParam? param);

Should it not be this?

typedef AsyncAction1<TParam> = Future Function(TParam param);

After all, action from above is passed to RxCommandAsync's _func, which is defined as

final Future<TResult> Function(TParam)? _func;

... same question for these two:

typedef Action1<TParam> = void Function(TParam? param);
typedef AsyncFunc1<TParam, TResult> = Future<TResult> Function(TParam? param);

Generally, we would not expect a null value to be returned from a command that is set up like RxCommand.createSync<bool, bool>(_func), or am I missing something?

Thanks for your feedback!

Edit: Looking at the API of flutter_command, this should be corrected to mirror the API of flutter_command more closely...

Answer 15 · 2021-04-23T10:42:17.000Z

you are probably right

Answer 16 · 2021-04-23T10:42:38.000Z

could you make a PR and try if it works? I'm kind a busy at the moment

Answer 17 · 2021-04-26T15:24:25.000Z

Hi,
Seems like something broken between migration to null safety. Here is code snippet.
var stringCommand = RxCommand.createAsync<String, String>((param) async => 'Here is your string - $param'); stringCommand.listen((value) { print('Value is not empty ${value.isNotEmpty}'); });
In this case value may be nullable for some reason, while params are non null.

Answer 18 · 2021-04-26T19:11:34.000Z

@5eeman This issue has already been identified and is addressed in PR #53

Answer 19 · 2021-04-28T04:00:01.000Z

Everything's looking good as far as I can see.

What we are left with is this "issue" here:

Hi all,

I have put together an initial PR for migrating the package to null-safety. However, I can't seem to find a way to migrate the package in a way that this

late RxCommand<bool, bool> myCommand;

[...]

RxCommand.createSync<bool, bool>(_myFunc)

[...]

bool void _myFunc(bool param) {
return bool;
}

[...]

myCommand();
myCommand.execute();

You would expect myCommand() to give you an error, because TParam is non-nullable, but it doesn't as execute and call are set up with a nullable TParam:

  /// Calls the wrapped handler function with an option input parameter
  void execute([TParam? param]);

  /// This makes RxCommand a callable class, so instead of `myCommand.execute()` you can write `myCommand()`
  void call([TParam? param]) => execute(param);

I've played around with this, but it seems there is no way to make TParam non-nullable and optional at the same time. So we either have to live with this or will have to go for explicitly calling myCommand(null) in cases where TParam is nullable.

I am okay to live with this. Any thoughts?

Answer 20 · 2021-04-28T07:18:48.000Z

Actually I would call this not a problem but a feature. It took me quite some work to get it work like this so that you don't have to pass null explicitly.
IMHO I prefer that I can still pass the command directly to event handlers in that which is even if this means they might get an exception when I don't pass a parameter for a non nullable.
At least as a design decision that I took for flutter_command

Answer 21 · 2021-04-28T07:20:23.000Z

Thanks a lot for all your help. Migrating a complex package like rx_command isn't easy without people testing and giving feedback!!!

Answer 22 · 2021-04-28T15:37:53.000Z

Thanks for the feedback. If it is a design decision that is okay, of course. I just thought I'd ask about it ;-)

Answer 23 · 2021-04-28T15:47:14.000Z

I think we leave the package another week in prerelease mode to see if we get any complaints

Answer 24 · 2021-05-07T17:57:27.000Z

I've successfully tested the null-safety release in our app's beta program without experiencing any issues. I'd suggest to go ahead with releasing...

Answer 25 · 2021-05-08T08:15:14.000Z

If we don't get any comments till monday I push a new release version

Answer 26 · 2021-05-22T10:48:24.000Z

@escamoteur , guess we don't have any comments besides updating docs PR. Please push new version. I'll use it for RxWidgets as well.

Answer 27 · 2021-05-26T10:46:42.000Z

Just pushed V6.0.0