search

schultek / dart_mappable

Improved json serialization and data classes with full support for generics, inheritance, customization and more.
https://pub.dev/packages/dart_mappable
MIT License
164 stars 23 forks source link

`includeCustomMappers` confusing global state behavior when used in MappableClass annotation #231

Closed AhmedLSayed9 closed 1 month ago

AhmedLSayed9 commented 2 months ago

Similar to #195, I got confused about using includeCustomMappers property at MappableClass and it was very hard to catch its global state behavior.

I don't get this idea of adding includeCustomMappers to MappableClass annotation while it'll change the global state for all other classed after that annotated class is called anyway.

That weird behavior caused a bug on my end that was very hard to catch. Specifically that includeCustomMappers api doc says: Specify additional custom mappers that should be included for fields of this class.. "this class" is meant to refer solely to its own class and should not be globalized to all other classes.

I suggest to either update it to just work for the class it's annotated with, or remove it from the class annotation and users can use MapperContainer.globals.use(CustomMapper()) instead.

schultek commented 1 month ago

Mappers are always registered globally. There cannot be two different mappers for the same type.

But I can make the docs more clear on this.

AhmedLSayed9 commented 1 month ago

Mappers are always registered globally. There cannot be two different mappers for the same type.

I meant when adding includeCustomMappers to some class annotation, it should only work for instances of that class type.

schultek commented 1 month ago

I understand. It would be a super huge change in how the package works and quite some work, which I don't have time for currently. So it won't be done anytime soon.

yehorh commented 1 month ago

Thank you for the wonderful package. The convenient API and functionality are impressive; it's excellent. 🤩

I had never used includeCustomMappers, but I decided to migrate a working project to this package and noticed from some tests that date serialization here is in UTC by default. I decided to change it globally, but unfortunately, you have to call a method to override DateTimeMapper.encodingMode, and this needs to be called in the code both before the tests and when starting the program. It's easy to forget, and it's impossible to set it by default at the code generation level 😔.

I decided to override it at the level of certain models using includeCustomMappers, and I realized that it works well in tests, but in production, it affects other classes where includeCustomMappers was not used.

It's very unexpected behavior.

schultek commented 1 month ago

I updated the docs to be more clear.

Unfortunately there is nothing else I can do about this.