search

rinukkusu / spotify-dart

A dart library for interfacing with the Spotify API.
BSD 3-Clause "New" or "Revised" License
206 stars 92 forks source link

feat: enable to json for every serializable model #213

Closed KRTirtho closed 5 months ago

KRTirtho commented 5 months ago

Closes #212

hayribakici commented 5 months ago

@KRTirtho that's a useful feature! Could you check the CI build for any errors?

rinukkusu commented 5 months ago

I think I/we have removed it to lower the warnings and better our dart analyzer score regarding unused code. Of course we can always revisit this topic. Currently I have no hard feelings regarding toJson or not toJson.

hayribakici commented 5 months ago

@rinukkusu is it possible to somehow "configure" whether a toJson should exist or not? Maybe through pubspec?

KRTirtho commented 5 months ago

Sorry for the late reply. I'll fix the CI issues asap. (Forgot to add @override for some toJson lol)

I think I/we have removed it to lower the warnings and better our dart analyzer score regarding unused code.

As far as I know, if unused methods are not private, it shouldn't decrease pub performance score. Because libraries are supposed to offer methods to the user which can unused by the library itself.

KRTirtho commented 5 months ago

Btw, in case you forgot to check, I fixed the CI issues 👍

rinukkusu commented 5 months ago

@KRTirtho released with v0.13.6

KRTirtho commented 5 months ago

That's great news 🎉

But, we have a issue with toJson of child property not being called by default. jsonEncode calls toJson on Object that are not primitve before calling toString, so we don't face any problem. But when we encode json without dart's default, we face the <Type Name> is not Map<String, dynamic> exception.

As you can see, .toJson is not called for externalUrls property for _$$AlbumToJson https://github.com/rinukkusu/spotify-dart/blob/1610dfd2aaddef4b76bca230dc923af084346937/lib/src/models/_models.g.dart#L68

Adding following in build.yaml fixes the issue

targets:
  $default:
    builders:
      json_serializable:
        options:
          any_map: true
          explicit_to_json: true

I'll create a PR with the simple fix. Sorry for the trouble, I didn't know this issue until I used Hive to cache some responses. Tbh, explicit_to_json should've been true by default. Not sure why it's turned off in the first place