Closed marcglasberg closed 7 months ago
Hey @marcglasberg, great feedback and thanks for writing this up.
My Discord comment probably should have said:
we need to know all the types statically so we can ensure runtime safety
It's true that we could allow Object?
/dynamic
for JSON primitives and simply throw when a non-primitive value was passed. Ultimately, though, this feels like a major footgun, especially for new developers. I had even debated removing Map<String, Object?>
support initially for the same reason.
I do think there is a model which can satisfy all uses cases coming with extension types, which would allow an explicit opt-in to this behavior without any runtime overhead. For example, you could have the following:
extension type JsonValue(Object? value) {}
class TranslatableUserException implements Exception {
factory TranslatableUserException(String message,
[Object? value1,
Object? value2,
Object? value3,
Object? value4,
Object? value5]) =>
TranslatableUserException._(message, JsonValue(value1), JsonValue(value2),
JsonValue(value3), JsonValue(value4), JsonValue(value5));
const TranslatableUserException._(this.message,
this.value1, this.value2, this.value3, this.value4, this.value5);
final String message;
final JsonValue value1, value2, value3, value4, value5;
}
While this is more verbose, it provides a level of type-safety that cannot be guaranteed with Object?
/dynamic
. It disincentivizes using those types at the client-server boundary but would enable a "I know what I'm doing" mechanism as well.
Thoughts?
This solution is great, I love it.
I was just reading this file: https://github.com/celest-dev/celest/blob/main/packages/celest_core/test/json_value_test.dart which uses JsonValue
JsonValue
currently has no unnamed constructor. Would you add it, so that you can write JsonValue(value1)
?
final class JsonObject extends JsonValue {
/// Creates a [JsonObject] from [wrapped].
const JsonObject(this.wrapped)
: _path = const [],
assert(
wrapped == null ||
wrapped is String ||
wrapped is int ||
wrapped is double ||
wrapped is bool ||
wrapped is List<Object?> ||
wrapped is Map<String, Object?>,
'Unsupported JSON value: $wrapped');
const JsonObject._(this.wrapped, [this._path = const []]);
@override
final Object? wrapped;
@override
final List<String> _path;
}
Yes! My goal was to make JsonValue
part of the public interface when extension types are released 😄 Currently, I don't feel like it's worth the overhead with the class-based version.
And I'll make sure to add a constructor like that for easy usage 💯
You mean https://github.com/dart-lang/language/issues/2727 ?
I personally don't mind the overhead, though.
Yes, that should be released in Dart 3.3 soon 👍
The overhead is really two-fold with classes vs. extension types
String
and JsonString
have entirely different identities.What I mean is:
// With classes
class JsonString extends JsonValue {
const JsonString(String value): super(value);
}
const String s = 'abc';
const JsonString js = JsonString(s);
print(js == s); // false
print(js is String); // false
// With extension types
extension type const JsonString(String value) implements JsonValue {}
const String s = 'abc';
const JsonString js = JsonString(s);
print(js == s); // true
print(js is String); // true
They are basically marker types which only exist at compile type, which means I can treat every JsonString
as a String
when it comes to serialization, equality checking, etc. Otherwise, I would need to implement a lot of boxing/unboxing in the code generator and have special paths carved out for these types.
It's doable, but since Dart 3.3 is right around the corner, I wanted to save myself the energy 😆
Perfect!
@dnys1 Dart 3.3 is out! :)
On it! 😄
The JsonValue
family has been released in 0.2.0 🚀
@dnys1 said (in Discord):
I already have a use case for this, as one of my exception types needs to have a few
Object?
fields:I'm not sure you do need to know all the types statically. Suppose your function is
doSomething(Object? value)
and you get one of these JSON values from the request:{ 123 }
or{ "abc" }
or{ true }
. Why can't you simply desserialize this asdoSomething(123)
,doSomething(true)
respectively?I understand you can't use any object there, but maybe allow for the common values accepted in JSON (plus Date?), and throw an error at runtime if an invalid value is used. In the future you can use an IDE plugin (https://github.com/celest-dev/celest/issues/34) to show an error in the code editor if an invalid value type is directly used.