Skip to content

Add option for implicitToJson #195

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

Add option for implicitToJson #195

wants to merge 5 commits into from

Conversation

kevmoo
Copy link
Collaborator

@kevmoo kevmoo commented May 28, 2018

Allows users to opt-in to including .toJson() calls on nested
objects in generated toJson methods.

This is useful if you want to round-trip the output of toJson without
putting it through jsonEncode and jsonDecode

Fixes #192

@kevmoo kevmoo requested review from jakemac53 and natebosch May 28, 2018 21:30
@kevmoo
Copy link
Collaborator Author

kevmoo commented May 28, 2018

CC @jmrboosties – let me know if this makes sense...

@jmrboosties
Copy link

Looks great! Thanks a lot.

@natebosch
Copy link
Member

I'm not sure about the naming... I don't think this is about "explicit" vs "implicit". Maybe something like nestedToJson?

Also, is there any reason not do do this by default?

@kevmoo
Copy link
Collaborator Author

kevmoo commented May 28, 2018

I'm not sure about the naming... I don't think this is about "explicit" vs "implicit". Maybe something like nestedToJson?

I was thinking that the call to toJson is implicit in dart:convert. This makes it explicit. Not sure nestedToJson is any more clear...

Also, is there any reason not do do this by default?

I pondered making this implicitToJson with a default of true (unlike the other options) and a note that we'll flip the default in the next release.

I think it makes the behavior a bit more clear...

jakemac53
jakemac53 reviewed May 29, 2018
View reviewed changes
json_serializable/lib/src/type_helpers/json_helper.dart Outdated
@@ -21,11 +21,15 @@ class JsonHelper extends TypeHelper {
/// By default, JSON encoding in from `dart:convert` calls `toJson()` on
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: update this comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By noting that if context.explicitToJson is true these calls will be injected automatically.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

jakemac53
jakemac53 reviewed May 29, 2018
View reviewed changes
json_serializable/test/src/json_serializable_test_input.dart

@JsonSerializable(createFactory: false)
class TrivialNestedNullable {
TrivialNestedNullable child;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider adding an int or some other non-custom type here, we want to make sure we don't call toJson on those

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure...the code paths for this are so crazy independent, I'm not too worried – but paranoia is good!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also prevents future regressions etc, looking at this PR alone its not clear this has the correct behavior today.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@kevmoo
Copy link
Collaborator Author

kevmoo commented May 29, 2018

@jakemac53 thoughts on changing the flag name/default?

We could do with implicitToJson and set it to true now and then change the default at a breaking change?

@natebosch ?

@jakemac53
Copy link
Collaborator

jakemac53 commented May 29, 2018 •
edited
Loading

I like explicitToJson, and eventually defaulting it to true I think. These things should be usable outside of the built in json encoding/decoding.

I actually ran into this with the firebase package if I recall... I think it doesn't use the built in json encode/decode functionality.

I would also be fine with implicitToJson, but I don't like nestedToJson. Maybe, implicitNestedToJson lol but that's just getting out of control ;).

kevmoo added 3 commits May 29, 2018 15:28
@kevmoo
Add option for explicitToJson
4e5608d 
Allows users to opt-in to including `.toJson()` calls on nested
objects in generated `toJson` methods.

This is useful if you want to round-trip the output of toJson without
putting it through jsonEncode and jsonDecode

Fixes #192
@kevmoo
include another feield in explicit toJson test
01e6694
@kevmoo
Flip the option name and default
be7394b
@kevmoo kevmoo force-pushed the i192_call_to_json_automatically branch from f31db59 to be7394b Compare May 29, 2018 23:35
@kevmoo
Copy link
Collaborator Author

kevmoo commented May 29, 2018

Name/default flipped. PTAL @jakemac53 @natebosch

@kevmoo kevmoo changed the title Add option for explicitToJson Add option for implicitToJson May 29, 2018
@natebosch
Copy link
Member

I still don't think "implicit" vs "explicit" is the distinction this is making. This is about an output which is only compatible with jsonEncode vs being valid json types. "shallow" vs "nested" is the distinction this is making. With a "shallowToJson" the nested fields are not converted to json types, with a "nestedToJson" the nested fields are converted to json types.

@kevmoo
Copy link
Collaborator Author

kevmoo commented May 30, 2018

I think the name is not super important. It'll be rarely used – just need to make sure it's documented.

I'm okay with shallowToJson: true – okay w/ you @natebosch ? @jakemac53 ?

@jakemac53
Copy link
Collaborator

jakemac53 commented May 30, 2018 •
edited
Loading

What about recursiveToJson? I am fine with shallowToJson or deepToJson also.

@natebosch
Copy link
Member

The other option is to skip the option, make it always on, and do the breaking version bump with this change. This package has a lot of configuration options already and it might not hurt to reduce the surface area a bit.

@kevmoo
Copy link
Collaborator Author

kevmoo commented May 30, 2018

The other option is to skip the option, make it always on, and do the breaking version bump with this change. This package has a lot of configuration options already and it might not hurt to reduce the surface area a bit.

  1. I'd rather hold off on this until the next release. We have a lot of stuff queued up.

  2. A nice thing we throw away here: checks for loops. dart:convert does the work to make sure we only visit Customer: hash 1234abcd once.

@kevmoo
Copy link
Collaborator Author

kevmoo commented May 30, 2018

Going to hold off on this PR for a bit...

@kevmoo kevmoo closed this May 30, 2018
@kevmoo kevmoo mentioned this pull request Jun 7, 2018
Merged
@Gopinathp Gopinathp mentioned this pull request Jan 14, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakemac53 jakemac53 jakemac53 left review comments

@natebosch natebosch Awaiting requested review from natebosch

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kevmoo @jmrboosties @natebosch @jakemac53