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

Support for json_serializable 5.0.0 #474

Closed
abibiano opened this issue Jul 14, 2021 · 21 comments
Closed

Support for json_serializable 5.0.0 #474

abibiano opened this issue Jul 14, 2021 · 21 comments
Labels
enhancement New feature or request needs triage

Comments

@abibiano
Copy link

With the publication of json_serializable 5.0.0 yesterday, is it planned to update freezed to use this release?

json_serializable >=5.0.0 depends on analyzer ^2.0.0 and freezed 0.14.2 depends on analyzer ^1.5.0, json_serializable >=5.0.0 is incompatible with freezed 0.14.2

@abibiano abibiano added enhancement New feature or request needs triage labels Jul 14, 2021
@SunlightBro
Copy link
Contributor

SunlightBro commented Jul 14, 2021

I tested with those updated dependencies:

analyzer: ^2.0.0
json_serializable: ^5.0.0
json_annotation: ^4.1.0

test are failing because json_serializable generates faulty *.g.dart files
For example /freezed/test/integration/export.dart

export.g.dart
// GENERATED CODE - DO NOT MODIFY BY HAND

part of 'export.dart';

// **************************************************************************
// JsonSerializableGenerator
// **************************************************************************

_$_Export _$$_ExportFromJson(Map<String, dynamic> json) => _$_Export(
      json['a'] as int,
    );

Map<String, dynamic> _$$_ExportToJson(_$_Export instance) => <String, dynamic>{
      'a': instance.a,
    };

The function name _$$_ExportFromJson should be _$_$_ExportFromJson (same for _$$_ExportToJson).

I'm not sure if this is an issue with json_serializable or analyzer. This needs more investigation.
But I think for now an upgrade to 5.0.0 is not viable.

@hpoul
Copy link
Contributor

hpoul commented Jul 15, 2021

@SunlightBro I don't think this is a faulty behavior, since the changelog contains:

Improve names of private classes generated for toJson and fromJson.

this seems to be intentional: google/json_serializable.dart@c72dd11

although I don't quite understand the reason for that change, since it was somehow related to freezed? 🤔️ google/json_serializable.dart#619

@SunlightBro
Copy link
Contributor

Ok I created a PR,
but I'm running into the issue that json_annotation 4.1.0 added @Target() annotations to minimize incorrect usage #912.

So @JsonKey should only be used on fields and getters but in freezed you annotate the parameters.
So @JsonSerializable should only be used on classes but in freezed you annotate the constructors.

Don't see a better way then just to add some

//ignore: invalid_annotation_target

And I don't really like that.

@hpoul
Copy link
Contributor

hpoul commented Jul 16, 2021

@SunlightBro maybe I'm wrong, but there is no real reason to use JsonKey from json_annotation to begin with.. freezed could provide it's own (Freezed)JsonKey (maybe subclass/implements the original?). If i understand it correctly, json_serializable always ignored the annotations on parameters, and only detected the ones in the code which freezed generates? So users would use freezed annotations and freezed generates the correct JsonKey annotations. 🤔

@SunlightBro
Copy link
Contributor

SunlightBro commented Jul 16, 2021

Yes but currently it's implemented using the annotations from json_annotation.
And I think your suggestion would introduce a breaking change, because that new

(Freezed)JsonKey

cannot be named the same.

@rrousselGit
Copy link
Owner

I will discuss with the meta team to see if the lint can be disabled when an annotation is applied on a Freezed constructor.

@rrousselGit
Copy link
Owner

https://github.com/dart-lang/linter/issues/2778

@creativecreatorormaybenot
Copy link
Contributor

@rrousselGit any ideas for an interim solution?

@Hari-07
Copy link

Hari-07 commented Jul 30, 2021

Is this resolved yet?

The function name _$$_ExportFromJson should be _$_$_ExportFromJson (same for _$$_ExportToJson).

@rrousselGit
Copy link
Owner

Closing since json_serializable 5.0.0 is supported

@creativecreatorormaybenot
Copy link
Contributor

@rrousselGit so the proposed solution is including ignores everywhere?

So let's put it like this: if we upgrade to json_serializable right now, our CI will break and we have to fix all of the warnings.

@hpoul
Copy link
Contributor

hpoul commented Aug 6, 2021

@creativecreatorormaybenot i guess technically your build breaks due to the json_serializable update, which is a major version change and hence breaks compatibility by definition 🤔

@rrousselGit
Copy link
Owner

@rrousselGit so the proposed solution is including ignores everywhere?

So let's put it like this: if we upgrade to json_serializable right now, our CI will break and we have to fix all of the warnings.

This issue was about supporting the version 5.0.0, which is now done.

The warnings needs to be tackled separately.

@rrousselGit
Copy link
Owner

But for now, I would suggest disabling the invalid_annotation_target warning inside your analysis_options.yaml

@creativecreatorormaybenot
Copy link
Contributor

@rrousselGit sounds good 👍 Is there an issue for tracking the warnings here? Or just https://github.com/dart-lang/linter/issues/2778?

@rrousselGit
Copy link
Owner

There is #488

But do note that considering the amount of work necessary to fix the warning, I probably won't work on it.
Disabling the warning is a good solution for now imo

@proninyaroslav
Copy link

proninyaroslav commented Aug 20, 2021

@rrousselGit
I'm still getting this bug in version 0.14.5:

The function name _$$_ExportFromJson should be $$_ExportFromJson (same for _$$_ExportToJson).

#474 (comment)

I had to go back to json_serializable 4.1.3.

proninyaroslav added a commit to proninyaroslav/libretrack that referenced this issue Aug 20, 2021
@proninyaroslav
Copy link

proninyaroslav commented Aug 20, 2021

@rrousselGit
UPD: very strange thing if i use json_serializable 4.1.3 and freezed 0.14.5. Then the bug with the name appears in the generated freezed.dart file, and not in the g.dart file).

I had to go back to freezed: 0.14.2 and json_serializable 4.1.3.

proninyaroslav added a commit to proninyaroslav/libretrack that referenced this issue Aug 20, 2021
@proninyaroslav
Copy link

@rrousselGit
After several attempts to rebuild the project and return all dependencies, everything returned to normal. But this is very strange.

@rrousselGit
Copy link
Owner

Sounds like you used either upgraded freezed without upgrading json_serializable, or did the opposite.

@proninyaroslav
Copy link

@rrousselGit
Initially, I upgraded both dependencies, and then this bug appeared. I tried to clean the project, lower the versions of both dependencies, but the problem disappeared is also strange as it appeared.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs triage
Projects
None yet
Development

No branches or pull requests

7 participants