Closed marcglasberg closed 4 months ago
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.
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.
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 JsonConverter
s. 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.
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:
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.
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?
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.
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 hasfromJson/toJson
methods you cannot control and which are causing errors with Celest. The simplest override of this type would look like this (defined inlib/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 originalMyType
when it encounters aMyType
value at any point. The override applies globally and affects all instances ofMyType
when passing from client<->server. And sinceMyTypeOverride
does not definefromJson
/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 inlib/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).
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).
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.
@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.
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!
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:
And this
toJson/fromJson
code is then used in a "local disk persistor" hereBut 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 validtoJson/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:
This doesn't work, because Celest will pick up the
toJson/fromJson
methods and will do this:This will result in a StackOverflow!
Here is a simple solution: instead of calling these methods
toJson
andfromJson
, I can call themtoJsonX
andfromJsonX
:Now Celest will correctly generate its default serializers, which I am using. And now I can use
toJsonX
andfromJsonX
in my persistor:This works, but I'd like to be able to name these methods
toJson
andfromJson
instead oftoJsonX
andfromJsonX
. 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 withtoJson
andfromJson
and it would be nice for Celest's serialization to be compatible.I propose one of these solutions:
toJson/fromJson
if we add an annotation to them, like@CustomSerializer
; OrtoJson/fromJson
if we add an annotation to them, like@CelestIgnore
; OrtoJson/fromJson
methods are using a CelestSerializer
subclass internally, it ignores them automatically.