Odonno / ReduxSimple

Simple Stupid Redux Store using Reactive Extensions
http://www.nuget.org/packages/ReduxSimple/
MIT License
144 stars 20 forks source link

Use readonly refs keyword `in` #33

Closed Odonno closed 4 years ago

Odonno commented 6 years ago

Following a discussion with @pguilbert I added readonly refs on the library to ensure immutability using the in keyword.

I now have questions about this addition:

I prefer not to introduce breaking changes by merging PR. Or if it does, it should be consistent with current developer tools.

EDIT: we may also use readonly struct on small classes like ReduxMemento if we switch to C#7.2

pguilbert commented 6 years ago

I can see it adds a dependency on the csproj 7.2. Does it requires developers to target >= 7.2 too? If not, does it requires developers to use the in keyword in their app?

I just made the test: you can reference a project with a different LangVersion and it's possible to use methods that have the inkeyword. Obviously it's impossible to use ref readonly to store reference return.

Regard to your changes, I think you will have to remove some in keywords inside "ReduxStore.cs", "ReduxStoreWithHistory.cs" (at least for public methods like Dispatch). Those in allows someone outside the store to change the value of the original object.

Exemple:

var action= new Action();
store.Dispatch(action)
action = new Action();
Odonno commented 6 years ago

Well, thanks for the feedback.

Good to know that we don't have to target the same C# version to use these features.

About the removal of in in some methods, I think it does not change the behavior of the library. I mean, on the internal parts, we ensure immutability (inside Dispatch/ObserveState/.. methods). But of course, we never know what can be done outside these methods.

Since actions are objects (references), you can already mutate the action object even if the result of this can be unpredictable.

var action= new Action(); // new action
store.Dispatch(action); // dispatch action
action = new Action(); // should be forbidden since it is not immutable (causing side effects on the store whether the `in` keyword is used or not

Maybe I miss something. And maybe writing unit tests could clarify my thoughts.

Odonno commented 6 years ago

@pguilbert What about a small review?