Incompatibility of custom toJson/fromJson with other non-Celest code

[marcglasberg]

Celest allows me to define custom serialization/deserialization by implementing toJson/fromJson in a model class.

That's fine until you have some other needs for these serializer methods. For example, in here I need to save the app state to the local device disk. To that end, I also use toJson/fromJson.

For example, here I have:

  Map<String, dynamic> toJson() => {
        'stocks': stocks.map((stock) => stock.toJson()).toList(),
        'cashBalance': cashBalance.toJson(),
      };

  factory Portfolio.fromJson(Json? json) {
      ...

And this toJson/fromJson code is then used in a "local disk persistor" here

Future<Portfolio> _readPortfolio() async {
    LocalJsonPersist localPersist = LocalJsonPersist(dbName_Portfolio);
    Object? result = await localPersist.load();
    return Portfolio.fromJson(result as Json?);
  }

Future<void> persistDifference({required AppState? lastPersistedState, required AppState newState}) async {
if (newState.portfolio != lastPersistedState?.portfolio) {
      var localPersist = LocalJsonPersist(dbName_Portfolio);
      await localPersist.save(newState.portfolio.toJson());
    }
}

But now, instead of creating its own serializers, Celest will use the toJson/fromJson I've created. That's a waste, as my goal here was not to customize the serialization, but only to provide my persistor with valid toJson/fromJson methods. Ideally, Celest should have written those serializer methods for me, and I must find a way to use them in my persistor too.

So, I'll try using Celest's serialization:

import 'package:celest_backend/src/client/serializers.dart';

Map<String, dynamic> toJson() => const PortfolioSerializer().serialize(this);
factory CashBalance.fromJson(Map<String, dynamic> value) => const PortfolioSerializer().deserialize(value);

This doesn't work, because Celest will pick up the toJson/fromJson methods and will do this:

final class PortfolioSerializer extends Serializer<Portfolio> {
  const PortfolioSerializer();

  @override
Portfolio deserialize(Object? value) {
    final serialized = assertWireType<Map<String, dynamic>>(value);
    return Portfolio.fromJson(serialized);
  }

  @override
  Map<String, dynamic> serialize(Portfolio value) => value.toJson();
}

This will result in a StackOverflow!

Here is a simple solution: instead of calling these methods toJson and fromJson, I can call them toJsonX and fromJsonX:

Map<String, dynamic> toJsonX() => const PortfolioSerializer().serialize(this);
factory CashBalance.fromJsonX(Map<String, dynamic> value) => const PortfolioSerializer().deserialize(value);

Now Celest will correctly generate its default serializers, which I am using. And now I can use toJsonX and fromJsonX in my persistor:

Future<Portfolio> _readPortfolio() async {
    ...
    return Portfolio.fromJsonX(result as Json?);
  }

Future<void> persistDifference({ required AppState? lastPersistedState, required AppState newState }) async {
      ...
      await localPersist.save(newState.portfolio.toJsonX());
  }

This works, but I'd like to be able to name these methods toJson and fromJson instead of toJsonX and fromJsonX. Also note that I have control over which methods my persistor uses, but that's not always the case. Some packages in the wild (like JsonSerializable) only work with toJson and fromJson and it would be nice for Celest's serialization to be compatible.

I propose one of these solutions:

1. That Celest only uses the custom toJson/fromJson if we add an annotation to them, like @CustomSerializer; Or
2. That Celest ignores the custom toJson/fromJson if we add an annotation to them, like @CelestIgnore; Or
3. If Celest detects that the custom toJson/fromJson methods are using a Celest Serializer subclass internally, it ignores them automatically.

[dnys1]

Hey @marcglasberg, thanks for the detailed issue. It brings to light a lot of the challenges around designing a framework like Celest and where to draw the lines of simple and opinionated. Please bear with me as I walk through my thoughts on these topics below.

Opinionated vs. Customizable

Celest aims to be an opinionated framework. What that means is that we aim for there to be a single way of doing things whenever possible, even at the cost of decreased customizability. Packages like json_serializable and others provide a wide array of customizations, flags, and features which can be daunting and overwhelming when you're just getting started. But advanced use cases drive the incremental expansion of these APIs. It's a constant battle, one that I think the Dart team handles spectacularly well, and one that we are still trying to feel out as we go.

toJson/fromJson

The fromJson/toJson override is an instance of us looking at the ecosystem and seeing the patterns that already exist so that we can blend in in the most intuitive way. The heuristic we landed on is: if there is no fromJson constructor, we generate one; if there is, we use that. And same for toJson. The logic for this was copied nearly verbatim from json_serializable.

The problem you're experiencing arises, IMO, from the fact that the semantics of toJson/fromJson are overloaded. Taken literally, they describe methods which serialize/deserialize objects to/from a JSON-compatible type, respectively. However, in most contexts it seems, they represent the mapping of an object to/from its wire protocol. That creates a disparity here where you would need two sets of methods to describe both intentions, since they are not the same. I would (presumptuously) argue this is not the norm.

It may be I am off base, and maybe toJson/fromJson are not the correct methods to be overloading. If that's true, then the solution in my mind would be to establish a different set of heuristic methods, perhaps serialize and deserialize.

However, yet another solution that stands out in this particular case, would be to call your methods for storage interop toStorage/fromStorage since this would better describe the semantic use of the methods vs. the literal work they perform.

Again, I believe this problem boils down to the overloading of the terms toJson/fromJson, but my (perhaps incorrect) understanding is that the Dart ecosystem has all centralized around these methods being semantically equivalent to serialize/deserialize in the context of the client-server boundary.

Resolving your issue

One sticking point with my argument is the following:

Some packages in the wild (like JsonSerializable) only work with toJson and fromJson and it would be nice for Celest's serialization to be compatible.

I don't have a good answer for that at the moment. JsonSerializable resolves this by allowing the decoration of serializable types with custom JsonConverters. I am not in love with this solution for Celest.

I believe, like #35, there could be a solution waiting with extension types. For example, taking a simplified version of the CashBalance class.

class CashBalance {
  const CashBalance(double amount);
  
  final double amount;

  factory CashBalance.fromJson(Json json) => CashBalance(json.asDouble('amount')!);
  Json toJson() => {'amount': amount};
}

If I wanted to use CashBalance in my API but leave the fromJson/toJson methods in place, I could use the following in its place.

extension type const ApiCashBalance(CashBalance balance) {}

When this type is used in a Celest API, Celest would treat it as a new type with no toJson/fromJson methods and create its own. Further, if I wanted to define new toJson/fromJson methods specifically for API serialization, I could leverage the same structure.

extension type const ApiCashBalance(CashBalance balance) {
  factory ApiCashBalance.fromJson(Map<String, Object?> json) {
    return ApiCashBalance(CashBalance(json['amount'] as double));
  }

  Map<String, Object?> toJson() => {'amount': balance.amount};
}

---

Apologies for the long comment, and I hope it brings up some points for further discussion! I am definitely open to changing my mind on the topic.

[marcglasberg]

The problem you're experiencing arises, IMO, from the fact that the semantics of toJson/fromJson are overloaded. Taken literally, they describe methods which serialize/deserialize objects to/from a JSON-compatible type, respectively. However, in most contexts it seems, they represent the mapping of an object to/from its wire protocol. That creates a disparity here where you would need two sets of methods to describe both intentions, since they are not the same.

Yes, you are right. When you say toJson the question is: JSON to do what? I was once working on a message system, and the message on the server needed to have sender and receiver fields, because the database contained the messages for all user pairs. But in the local user device we only needed to save the one that was not the user (since here the other part must be the user himself). So, there were different toJson methods in the server and in the local device. Since the server was a separate system in TypeScript this was irrelevant. But with Celest we are sharing code, so trying to share the toJson is a possibility.

It brings a larger point that I was afraid when I started creating the demo app: That using the same classes in the frontend/backend may be "overloading" as well. I was afraid that sharing classes like Portfolio and CashBalance would not be possible because the requirements of those classes would end being very different in the backend and in the frontend. To an extend that's true, for example with the Portfolio class I have a buy method that's only used in the backend. I was afraid I'd need to create an abstract Portfolio class, with PortfolioClient and PortfolioServer subclasses to use them in the client/server respectively.

But I digress. When the question is "JSON to do what?" there is a toJson-ForCelest and a toJson-ToStorage. They may be the identical, similar, or completely different. And you convinced me to keep them separate.

Two final notes:

1. Most of the time they will be identical, but I like that the code is explicit when I write this:

   Map<String, dynamic> toStorage() => const PortfolioSerializer().serialize(this);

   Here it's explicit that I'm using Celest's serializer. I'd just like to ask you to always keep Celest serializers public! Don't change their names to _PortfolioSerializer because I'll depend on them. It would be nice to make them part of the public API by explaining that you can use them, in the Celest documentation.

2. While I'm satisfied with this solution for my specific current use case, I suppose Celest is potentially incompatible with JsonSerializable. People care a lot about that package, and have contributed code to make my Fast Immutable Collections package compatible with it. See here. I am not myself a JsonSerializable user, so I can't say more. But I'm sure someone else will open an issue if there are any problems with it, in the future.

May I close this issue?

[dnys1]

Good points. I'd like to think on this some more before closing it. I would love to not have a situation where client and server need distinct types.

[dnys1]

Hey @marcglasberg, I've stumbled upon a pretty clean solution to this. Let me know what you think! I've added tests for all your types into my CLI suite to make sure I've covered all the cases.

The solutions all revolve around a feature called "custom overrides" which is available in the latest dev release of Celest. Here's my spiel for the changelog:

There are times when you want to use a type from a third-party package in an API, but it is not serializable and Celest complains when you do so. Because you do not control the package, you cannot make it serializable and you are stuck. Other times, the opinionated way Celest serializes (e.g. by using fromJson/toJson methods if available) doesn't align with how you've defined these APIs.

In both of these situations, custom overrides allow you to reimagine the types so that they can be serialized with Celest.

Consider a type called MyType which has fromJson/toJson methods you cannot control and which are causing errors with Celest. The simplest override of this type would look like this (defined in lib/models/overrides.dart):

import 'package:some_package/some_package.dart';

@override
extension type MyTypeOverride(MyType _) implements MyType {}

The @override annotation tells Celest to use this type instead of the original MyType when it encounters a MyType value at any point. The override applies globally and affects all instances of MyType when passing from client<->server. And since MyTypeOverride does not define fromJson/toJson methods, Celest will generate its own.

Custom overrides can do a lot more than override fromJson/toJson methods, though. They can redefine constructors, fields, and even field types. And custom overrides work for exception types too (define those in lib/exceptions/overrides.dart)!

Here is how I've done the overrides for your model types:

import 'package:_common/marcelo.dart' as models;

@override
extension type AvailableStock(models.AvailableStock _)
    implements models.AvailableStock {}

@override
extension type AvailableStocks(models.AvailableStocks _)
    implements models.AvailableStocks {}

@override
extension type CashBalance(models.CashBalance _)
    implements models.CashBalance {}

@override
extension type Portfolio(models.Portfolio _) implements models.Portfolio {}

@override
extension type Stock(models.Stock _) implements models.Stock {}

@override
extension type Ui(models.Ui _) implements models.Ui {}

For the exception types, things get a bit more interesting, because many of them are not naturally serializable. For example, many have fields of type Object? which Celest doesn't allow. For these, though, you can either redefine the field or add a custom fromJson/toJson implementation to the override type. Here's an example of both approaches:

@override
extension type AppError(core.AppError _err) implements core.AppError {
  AppError.fromJson(Map<String, Object?> json)
      : _err = core.AppError(json['msg'], json['error']);

  Map<String, Object?> toJson() => {'msg': _err.message, 'error': _err.error};
}

@override
extension type AppException(core.AppException _ex)
    implements core.AppException {
  JsonValue? get error => _ex.error as JsonValue?;
  JsonValue? get msg => _ex.msg as JsonValue?;
}

If a fromJson/toJson type is not provided, then all the public fields and all the parameters of the unnamed constructor must be serializable. Such is not the case with UserException, for example. The fields can be overridden as above, but the function-typed parameters unnamed constructor prevents Celest from generating a serializer.

In this case, you can either write your own fromJson/toJson or redefine the unnamed constructor via the extension type where all parameters are serializable:

@override
extension type UserException._(core.UserException _ex)
    implements core.UserException {
  UserException({
    String? msg,
    JsonValue? cause,
  }) : this._(core.UserException(msg, cause: cause));

  Null get code => null;
  JsonValue? get cause => _ex.cause as JsonValue?;
}

In this case, I chose to redeclare the ExceptionCode field as simply Null since ExceptionCode cannot be serialized (it's an abstract class).

[dnys1]

Also note that these override types are only ever used by Celest when serializing. For example, defining an override on UserException means than anytime the original UserException is thrown it will be serialized via the override. This means that override types never need to be used directly (although they can be).

[dnys1]

This change has been released in 0.2.0 🚀

Let me know if it's a good solution for your issue! Happy to spend more time exploring alternatives.

[marcglasberg]

@dnys1 Yes, it's a really good solution, I love it! Congrats!

One minor thing is, why are you using @override annotation, and not your own @celestOverride or something? Since @override is already used for other reasons in Dart, developers with little experience with Celest may easily think it's not connected to Celest. They will think it's an error and remove it, and then the code will start failing at runtime, with no possible compile-time warning. One day, even the Dart linter can start complaining about it, because it's being used where it was not suppose to.

[dnys1]

Yeah, that could definitely be true. I thought override was the perfect word to describe what it does and loved that it aligned with the semantics of @override. I'm not too concerned about them breaking @override since it's in dart:core.

I like @celestOverride too, though, and agree it may help reduce confusion.. Let me consider that!

Thanks for bringing it up and I'm glad you are enjoying the feature! 🥳

I will close this issue for now, but will follow up with you on the @override annotation. Cheers!