Allow changing the name of toMap/toJson/fromMap/fromJson methods

[MikeFP]

A configuration for the name for decode and encode generated methods would be nice.

In one of my projects, I prefer using a typedef Json instead of Map<String, dynamic>, and it's really awkward having to use fromMap() and toMap() instead of fromJson() and toJson().

Besides, when I migrated it from freezed to dart_mappable, I had to rename every fromJson() to fromMap(). That was also kind of dangerous, considering that, in some places, I parsed a dynamic value from a local database, and the compiler couldn't warn me about the type, so I missed a couple of fromJson() from freezed - which expect to receive a Map - and the app broke at runtime when they actually expected a String with the method generated from dart_mappable.

With that configuration, I could've changed the names like this:

• fromMap / toMap -> fromJson / toJson
• fromJson / toJson -> deserialize / serialize

That naming convention would be easier for newcomers to grasp, or even for people used to working with freezed, for instance. Besides, I don't think there are many usecases where someone would work with JSON objects in Dart and not use a Map. The stringified JSON version is mostly used when transferring data over the network, I think.

Having the ability to change those names would be really useful.

[schultek]

I see your point. By now its almost a convention in dart to have fromJson take a map.

But I've never liked or supported it for the following reason:

While you will never have json without a map, the reverse is not the case. You might deal with maps without having anything todo with json. You might transform the maps to a totally different protocol, like xml, yaml, or some binary or even proprietary protocol. In those cases its just plain wrong to call it toJson.

[MikeFP]

The current naming is fine and is more generic, like you said. That's totally valid, that's why I would prefer a setting to change it for some projects only.

[schultek]

I added this as a build option for build.yaml in version 3.0.0-dev.5:

global_options:
  dart_mappable_builder:
    options:
      renameMethods:
        fromJson: deserialize
        toJson: serialize
        fromMap: fromJson
        toMap: toJson

I will leave this undocumented for now, but keep the issue open.

[RandalSchwartz]

I see this as a much more important issue. jsonEncode expects .toJson to work a certain way: return a Dart object that can be handed to the encoder with no further interaction (Map, List, num, bool, null, String). Your package violates that. I'd have to add your undocumented feature in any project where I might possibly use this package, just to conform to longstanding codes.

It's too bad you just rolled 3.0. This would be a breaking change to fix it to conform to standards, but until this is fixed, I cannot recommend the package.

[schultek]

I'm sorry you see it that way. Thing is you don't even need to call jsonEncode because .toJson() already outputs the encoded json string.

Would it make a difference if this is documented?

[RandalSchwartz]

I'm sorry you see it that way. Thing is you don't even need to call jsonEncode because .toJson() already outputs the encoded json string.

It's not me that's calling it. The jsonEncode calls .toJson when I encode a data structure. If it gets to something it doesn't immediately know, it calls .toJson on it, and it is expecting that to be a dart object, not already an encoded string, so it will end up encoding the whole value as a single quoted string within the hierarchy.

You're breaking protocol. Things will break.

[schultek]

I know how jsonEncode works. What I'm saying is that you don't need to do jsonEncode(myObject) and instead can directly do myObject.toJson() which will give you the desired output of a serialized json string.

[clragon]

Unfortunately, thats not a decision we can always make. There are numerous libraries that adopt the standard behavior set by jsonEncode (because they encode the object inside of their code themselves). Those libraries often do offer ways to specify different methods, but that would be cumbersome to adjust everytime.

Adhering to the standard would ease interoperability and make this package even more useful by default.

[schultek]

I see. I haven't thought about that to be honest.

Maybe giving in on this really is the way forward, in the end I want the package to be as usable as possible. Even if that means adhering to standards I personally don't feel good about.

[RandalSchwartz]

It is unfortunate that .toJson has come to be the equivalent of .toMap, expected to be a Dart structured object, not a String. But we're stuck with that for now, and lots of literature pretty much entrenches it.

[eitanliu]

I have implemented a temporary solution to this issue using the DartMappable plugin.

From example

{
  "float": 1.0,
  "string": "str"
}

It will generate the content:

@MappableClass()
class AaEntity {
  double float;
  String string;

  AaEntity(this.float, this.string);

  factory AaEntity.fromJson(Map<String, dynamic> map) =>
      AaEntityMapper._guard((c) => c.fromMap<AaEntity>(map));

  factory AaEntity.fromString(String json) =>
      AaEntityMapper._guard((c) => c.fromJson<AaEntity>(json));

  Map<String, dynamic> toJson() {
    return AaEntityMapper._guard((c) => c.toMap(this));
  }

  @override
  String toString() {
    return AaEntityMapper._guard((c) => c.toJson(this));
  }

  ///...
}

[schultek]

@eitanliu Please don't use _guard, this is an internal implementation that might change any time.

[eitanliu]

@eitanliu Please don't use _guard, this is an internal implementation that might change any time.

@schultek If there is support for custom methods, will the fromJson and toJson methods of the Mapper change, as I am uncertain about the modifications required for the supported outcome? If there are no modifications, I will call the methods of the Mapper accordingly.

[schultek]

I added this as a build option for build.yaml in version 3.0.0-dev.5:

global_options:
  dart_mappable_builder:
    options:
      renameMethods:
        fromJson: deserialize
        toJson: serialize
        fromMap: fromJson
        toMap: toJson

I will leave this undocumented for now, but keep the issue open.

Depends if the user has configured this.

[eitanliu]

Is there methods that does not change based on the configuration? Users may modify the configuration at any time, resulting in the nonexistence of old methods, need to replace all methods. However, the functionality has already been implemented, this solution is no needed.

[schultek]

Only methods in the generated code will change.

Other methods, like MapperContainer.globals.fromJson can't be changed.

[Tienisto]

It would help many users if you add this configuration to the README for effective json_serializable migration.

I nearly gived up migrating a mid-sized project to dart_mappable but fortunately, I found this issue.

The problem is that this error is silent (sometimes toJson is called explicitly, sometimes not), developers generally expect this implicit behaviour. Fortunally, this behaviour can be configured globally, otherwise, the migration process would be very time consuming (and might introduce many regression bugs)

I know, this is bad API design by the Dart team but that's how it is.

[schultek]

Thanks for bringing this to my attention again.

I am pretty fixed by now on keeping the naming as it is, since I have also received lots of feedback from devs that specifically like the proper naming.

But being how it is, I definitely recognize the potential gotchas this causes and want to make migration from json_serializable as easy as possible. So I will add this more visibly to the readme and expand the docs regarding migration to include the renaming option.

I will keep the issue open anyway so the discussion around this topic stays visible.

[rydmike]

Regarding the toMap/toJson/fromMap/fromJson method names

I struggled with it a bit here too https://github.com/schultek/dart_mappable/pull/155#issuecomment-1962128391

As can be seen to make dart_mappable work with retrofit and chopper, I tried the suggested config:

global_options:
  dart_mappable_builder:
    options:
      renameMethods:
        fromJson: deserialize
        toJson: serialize
        fromMap: fromJson
        toMap: toJson

But I was still getting the retrofit requires toJson() to return a map error.

For everything to work, and yes this applies when using freezed too, you have to configure what should be built in what order in the build runner's build.yaml file. This is required for the dependencies to be available in needed order. I ended up using the following configuration:

global_options:
  freezed:
    runs_before:
      - json_serializable
      - retrofit_generator
      - chopper_generator
  json_serializable:
    runs_before:
      - retrofit_generator
      - chopper_generator
  dart_mappable_builder:
    runs_before:
      - retrofit_generator
      - chopper_generator
    options:
      caseStyle: snakeCase # fits the target imaginary API
      ignoreNull: false
      discriminatorKey: type
      generateMethods: [decode, encode, copy, stringify, equals]
      # Use the rename options feature to make the generated code 
      # compatible with retrofit and chopper
      renameMethods:
        fromJson: deserialize
        toJson: serialize
        fromMap: fromJson
        toMap: toJson

Sure, I do not need or use freezed or json_serializable when using dart_mappable anymore, but the above config was from a test with them all, just to see that they can all work with retrofit and chopper packages.

The test also gave me a quick comparison of the syntax and feel of using these options. Personally I think dart_mappable has the most "Dart" like syntax, and in addition to its other benefits, this is also a big plus in my book.

You can find my simple test setup repo here, if it is of interest: https://github.com/rydmike/mapper_issue

Concern from the docs

What does concern me a bit, well actually a lot, is the comment in the documentation that states that using this config is not recommended and that might go away at any time at the whim of package author. As shown here:

I agree that dart_mappable has the more semantic correct idea of what from/to Map and Json are, however the prevailing standard for it is governed by prior art set by json_serialiazable. It might be semantically wrong, but it is what we got and many other packages expect and needs those methods.

In my opinion dart_mappable docs should instead consider doing the reverse and recommend to use these settings when working with packages like retrofit and chopper that needs from/to Json to work the way established by json_serializable. It should also not threaten to take away this support in future version. Well sure it can do what it likes of course, but it would also make this otherwise wonderful package useless to many projects.

To some projects and organizations, just the documented statement this configuration option may go away, might be enough for them to avoid this package. Which is sad, because it is a really great option to freezed and json_serializable.

It would also be nice if docs would mention the needed build runner order config for it to work. Not everybody is an expert on that runner monster. Sure it was not impossible to figure out, but it would have helped a lot and saved time, if the docs would have mentioned it.

Anyway, thanks for this great package! Loving it much more than the current other options 💙

[schultek]

Thanks for this very valuable feedback.

It helps me a lot to understand how people are using the package. I your particular I wasn't much aware the renaming feature is not only used for migration but also for compatibility with other package.

I will go ahead and remove the mentioned sentences from the docs, as I have no plans to remove support for this. Fyi I'm anyways working on a new version anyways where the user can define completely custom coding adapters to support not only Map and Json, but any serialization format (so things like toXml, toWhatever).

[schultek]

@rydmike See the updated docs: https://pub.dev/documentation/dart_mappable/latest/topics/Migration%20and%20Compatibility-topic.html

[rydmike]

That's a nice doc update @schultek, thank you. It looks and feels much better when the differences and need to support the json_serializable expectation is recognized, and a way offered to do so is presented 👍🏻 😄

[agent515]

I'm using Retrofit so I created build.yaml and updated it as per the Migration and Compatibility Document.

global_options:
  dart_mappable_builder:
    runs_before:
      # list the generator packages you depend on, e.g.
      - retrofit_generator
    options:
      # ... other dart_mappable options
      renameMethods:
        fromJson: fromJsonString
        toJson: toJsonString
        fromMap: fromJson
        toMap: toJson

It did rename methods in the generated *.mapper.dart as expected. But the generated retrofit file is still throwing The method 'fromJson' isn't defined for the type *. The reason being that it is using MyClassDto.fromJson(i as Map<String, dynamic>) as opposed to MyClassDtoMapper.fromJson() which dart_mappable generates.

How to get around this issue?

PS: I'm also using Freezed for union classes for models/ entities (it's first time I'm trying out dart_mappable) but it does not intervene with Data Classes.

[trejdych]

you can add fromJson to your model. Either a factory constructor or a static method

factory MyClassDto.fromJson(Map<String, dynamic> json) =>
      MyClassDtoMapper.fromJson(json);

static const fromJson = MyClassDtoMapper.fromJson;    

[agent515]

@trejdych That worked! Thanks!