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

[dart-next] Refactor to decouple serialization from networking #15485

Closed
wants to merge 35 commits into from

Conversation

ahmednfwela
Copy link
Contributor

@ahmednfwela ahmednfwela commented May 11, 2023

Introduce a new dart-next generator.

fixes #15477
as discussed in the above issue, this PR completely decouples the networking layer from the serialization/deserialization layer.
It also minimizes the serialization/networking layer's footprint, to simplify adding new serialization/networking options (like freezed #13047, or http)

It introduces new classes

  • TypeInfo - similar to built_value's FullType, but adapted to all serializers
  • SerializationRepositoryBase - the base class to handle all serialization operations in the API layer
  • BuiltValueJsonRepository - built_value's implementation of SerializationRepositoryBase
  • JsonSerializableRepository - json_serializable's implementation of SerializationRepositoryBase
  • a Raw version of each API that is decoupled from serialization/deserialization, and can handle any data format (not only json)

PR checklist

  • Write the repository implementations
  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • In case you are adding a new generator, run the following additional script :
    ./bin/utils/ensure-up-to-date.sh
    
    Commit all changed files.
  • File the PR against the correct branch: master (6.3.0) (minor release - breaking changes with fallbacks), 7.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@jaumard (2018/09) @josh-burton (2019/12) @amondnet (2019/12) @sbu-WBT (2020/12) @kuhnroyal (2020/12) @agilob (2020/12) @ahmednfwela (2021/08) @bahag-chandrana

…nto separate directories

WIP serialization repo implmentation
@ahmednfwela
Copy link
Contributor Author

ahmednfwela commented May 13, 2023

Update:

  • Introduced new CLI option serializationLibrary which takes either built_value or json_serializable
  • Replaced library CLI option to take either dio or http
  • I restructured the mustache files to decouple serialization and networking files
    The mustache files currently look like this:
    • dart2 (for the dart generator, unchanged)
    • dart (for the dart-dio generator)
      • libraries
        • dio
        • http
      • serialization
        • built_value
        • json_serializable
      • shared files between all options ...

The plan is that after this PR, we split the dart generator files to be under the libraries/http folder and serialization/native
which will be done in #14346
until then, I copied over the files from dart2 folder to libraries/http folder

@ahmednfwela ahmednfwela changed the title [dart-dio] Refactor to decouple serialization from networking [dart-next] Refactor to decouple serialization from networking Jun 17, 2023
@ahmednfwela
Copy link
Contributor Author

@kuhnroyal I have updated the PR to introduce a new generator dart-next

* use much newer exec plugin
* run pub via dart sub command
{{#imports}}import '{{.}}';
{{/imports}}

part '{{classFilename}}.g.dart';
Copy link
Contributor

Choose a reason for hiding this comment

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

This currently fails when there are no inline enums

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I am aware of that, and I am working on a fix rn and completing the generator changes

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, ill see if I have time to test during the FlutterCon days.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ahmednfwela Do you have a fix for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kuhnroyal yes, but I am a little busy for a couple of days

Copy link
Contributor

Choose a reason for hiding this comment

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

Can I help here in anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am pushing a fix for it today , sorry for the delay

@ahmednfwela
Copy link
Contributor Author

ahmednfwela commented Aug 2, 2023

I pushed a fix for the part files, what's currently blocking the PR is that built_value doesn't support const constructors yet
see
google/built_value.dart#1249
google/built_value.dart#1250
google/built_collection.dart#164
google/built_collection.dart#283

Also some work needs to be done on the nullability areas.

@kuhnroyal
Copy link
Contributor

I loosely followed the built_value tickets. I assume this is regarding default values? Can we move forward without those for now?

@ahmednfwela
Copy link
Contributor Author

@kuhnroyal we can just use normal dart primitives for the api definations, and only use built_value for models

@NotHolst
Copy link

NotHolst commented Aug 29, 2023

What is the point of supporting built list and built value?
I don't see what they offer over something like freezed or simply json_serializable
This is a genuine question, i'm very curious :)

@ahmednfwela
Copy link
Contributor Author

@NotHolst well, built_value was the first immutability library to come out for dart, but now that dart has grown up, it's starting to fall behind.

one of its perks though is the separation of serializers and models into different classes, which allows a plugin style for serialization.

@NotHolst
Copy link

@ahmednfwela okay, sounds like a niche use for most of us who just want api codegeneration that works out of the box.
I would suggest replacing the default with either json_serializable or freezed whenever they are stable. Working with these extremely verbose data classes can be confusing to anyone who don't understand or even need the benefits :)

@ahmednfwela
Copy link
Contributor Author

sounds like a niche use for most of us who just want api codegeneration that works out of the box.

@NotHolst I am honestly starting to go this way too, I have been arguing with built_value maintainers over adding const constructors for some time, and they don't seem to agree.

so I will be removing built_value support for the new generator and focus on other commonly used serialization packages, like json_serializable and freezed.

const SerializationRepositoryBase();

FutureOr<Object?> serialize<T extends Object?>(T src, TypeInfo inputTypeInfo, {Object? context,});
FutureOr<T> deserialize<T extends Object?>(Object? value, TypeInfo targetTypeInfo, {Object? context,});
Copy link
Contributor

Choose a reason for hiding this comment

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

Making these async seems overkill and increases complexity in other places.
What is your use case here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kuhnroyal you might want to offset the (de)serialization process to another isolate like the dart generator does

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Does it make sense to provide both options so that we can use the sync access especially for parameter serialization etc.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kuhnroyal that was actually my plan since I sometimes need the serialization logic in places where I can't use async

required EnumQueryDoubleEnum enumQueryDouble,
required BuiltList<ModelEnumClass> enumQueryModelArray,
BuiltList<InnerEnum> enumFormStringArray = r'$',
EnumFormStringEnum enumFormString = r'-efg',
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to find a way to generate these defaults correctly.
And also collection default values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with built_value, you can't since you need const constructors, so I plan on removing support for built_value entirely and focus on json_serializable/freezed

}


class InnerEnum extends EnumClass {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing its values, not sure why.

// ignore_for_file: unused_element
import 'package:openapi/src/model/fruit_type.dart';
import 'package:openapi/src/model/apple_any_of_disc.dart';
import 'package:openapi/src/model/banana_any_of_disc.dart';
Copy link
Contributor

Choose a reason for hiding this comment

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

These 2 imports do not exist, something is wrong with the anyof generation.

@NotHolst
Copy link

any updates to this?
is there anything we can do to help?

@NotHolst
Copy link

any updates to this? is there anything we can do to help?

@ahmednfwela

@ahmednfwela
Copy link
Contributor Author

@NotHolst Sorry I have been extremely busy lately with my professional work, that I don't have that much time to spare on opensource projects, I will however try to finish this PR since we will have to depend on it eventually.

@NotHolst
Copy link

NotHolst commented Dec 19, 2023

@ahmednfwela

I'm following up on this a month later to make sure it doesn't go stale.
Is there anything I or anyone else can do to expedite this process.

I'm willing to put in the work, but i'll need a briefing on current progress, goals/milestones and potential roadblocks.

@oravecz-jpmc
Copy link

Also, would like to know what is outstanding to move this forward?

@wing328
Copy link
Member

wing328 commented Jun 12, 2024

@ahmednfwela when you've time, can you please PM me via Slack? Thanks.

@ahmednfwela
Copy link
Contributor Author

Hello everyone following this PR, I have started a new PR that contains a better version of this generator that has no 3rd party dependencies for serialization/modelling, thus avoiding all the blocking I have faced.

I will be closing this PR in favor of:
#18970

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

Successfully merging this pull request may close these issues.

[REQ] [dart-dio] Refactor api methods and classes
5 participants