Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Why is explicitToJson not default? #620

Closed
rrousselGit opened this issue Mar 15, 2020 · 6 comments
Closed

Why is explicitToJson not default? #620

rrousselGit opened this issue Mar 15, 2020 · 6 comments

Comments

@rrousselGit
Copy link
Contributor

rrousselGit commented Mar 15, 2020

Hello!

Some background around the question:
I'm made a code-generator that optionally builds on the top of json_serializable (Freezed)
The JsonSerializable annotation is added by Freezed, not manually by its users (although users can override this with their own).

Now, the thing is, Freezed generates the default @JsonSerializable(), with no fancy parameter.
But a few of its users have faced issues because explicitToJson is not the default behavior.

Since I am the one who generates the annotation by default, I've been thinking of generating @JsonSerializable(explicitToJson: true) instead.
But this leaves me wondering: Surely there must be a reason why this flag is not enabled default.
But what is that reason?

I would like to make sure that doing such a change would not have undesired effects.

@kevmoo
Copy link
Collaborator

kevmoo commented Mar 15, 2020

See answers to

#561
#570

@kevmoo kevmoo closed this as completed Mar 15, 2020
@rrousselGit
Copy link
Contributor Author

rrousselGit commented Mar 15, 2020

I've seen these, but they do not answer my question.

Sure, we get lazy encoding (which we would get with a MapView/ListView too by the way). But why is it the default behavior?

Is this behavior only for lazy encoding? Are there any risk in disabling explicitToJson by default?

If this is only for lazy encoding, couldn't we instead use a MapView? Such that the output of our toJson correctly outputs only maps/lists/nums/strings, while being lazy too.
That's how decoding works with dart2js and the JsonMap/JsonList thing

@rrousselGit
Copy link
Contributor Author

More specifically, assuming this is only about lazy-loading, then we could add a LazyToJsonMapView on json_annotation (implementation at the end)

Then, instead of explicitToJson and generating:

return {
  'object': object
};

vs

return {
 'object': object.toJson()
};

Then we could have a lazyEncoding flag and generate:

return {
 'object': LazyToJsonMapView(object.toJson),
};

vs

return {
 'object': object.toJson(),
};

This would allow having that lazy-encoding behavior, but without having to change the "type" of the output, which should work with all encoders.

The implementation of LazyToJsonMapView would be:

class LazyToJsonMapView implements Map<String, dynamic> {
  LazyToJsonMapView(this._toJson);

  final Map<String, dynamic> Function() _toJson;
  Map<String, dynamic> _cache;
  var _computed = false;
  Map<String, dynamic> get _map {
    if (!_computed) {
      _computed = true;
      _cache = _toJson();
    }
    return _cache;
  }

  // Copied from MapView

  @override
  Map<RK, RV> cast<RK, RV>() => _map.cast<RK, RV>();
  @override
  dynamic operator [](Object key) => _map[key];
  @override
  void operator []=(String key, dynamic value) {
    _map[key] = value;
  }

  @override
  void addAll(Map<String, dynamic> other) {
    _map.addAll(other);
  }

  @override
  void clear() {
    _map.clear();
  }

  @override
  dynamic putIfAbsent(String key, dynamic Function() ifAbsent) {
    return _map.putIfAbsent(key, ifAbsent);
  }

  @override
  bool containsKey(Object key) => _map.containsKey(key);
  @override
  bool containsValue(Object value) => _map.containsValue(value);
  @override
  void forEach(void Function(String key, dynamic value) action) {
    _map.forEach(action);
  }

  @override
  bool get isEmpty => _map.isEmpty;
  @override
  bool get isNotEmpty => _map.isNotEmpty;
  @override
  int get length => _map.length;
  @override
  Iterable<String> get keys => _map.keys;
  @override
  dynamic remove(Object key) => _map.remove(key);
  @override
  String toString() => _map.toString();
  @override
  Iterable<dynamic> get values => _map.values;

  @override
  Iterable<MapEntry<String, dynamic>> get entries => _map.entries;

  @override
  void addEntries(Iterable<MapEntry<String, dynamic>> entries) {
    _map.addEntries(entries);
  }

  @override
  Map<K2, V2> map<K2, V2>(
    MapEntry<K2, V2> Function(String key, dynamic value) transform,
  ) {
    return _map.map<K2, V2>(transform);
  }

  @override
  dynamic update(
    String key,
    dynamic Function(dynamic value) update, {
    dynamic Function() ifAbsent,
  }) {
    return _map.update(key, update, ifAbsent: ifAbsent);
  }

  @override
  void updateAll(dynamic Function(String key, dynamic value) update) {
    _map.updateAll(update);
  }

  @override
  void removeWhere(bool Function(String key, dynamic value) test) {
    _map.removeWhere(test);
  }
}

@kevmoo
Copy link
Collaborator

kevmoo commented Mar 16, 2020

We had something like the lazy thing you're talking about, but we removed it – it was underused and added a lot of complexity to the code.

Right now, we're being very conservative about changes to this package. Changing the default is a BIG change.

@guplem
Copy link

guplem commented Jun 8, 2023

I am adding this build.yaml that I got from here in every project to make it the default:

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

@masus04
Copy link

masus04 commented Sep 12, 2023

For now I'm adding JsonSerializable(explicitToJson: true) to any serializable types that nest other serializable types, which is quite annoying but works.

gunantosteven pushed a commit to gunantosteven/admin that referenced this issue Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants