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

[REQ] [dart-dio] Refactor api methods and classes #15477

Open
ahmednfwela opened this issue May 11, 2023 · 10 comments
Open

[REQ] [dart-dio] Refactor api methods and classes #15477

ahmednfwela opened this issue May 11, 2023 · 10 comments

Comments

@ahmednfwela
Copy link
Contributor

Is your feature request related to a problem? Please describe.

currently dart-dio

  1. generates api methods that handle both serialization and networking (see [BUG][dart-dio] Accept header not generated #15427 (comment) for further explaination)
  2. generates api classes are tightly coupled to the serializer, which means that different serialization options have to change networking mustache files directly, leading to complexities mentioned in [REQ] [dart-dio] [internal refactor] Unify test schemas #15449 (comment)
    e.g.:

Describe the solution you'd like

We can mitigate both of these issues with some non-breaking refactor of the generated API classes, essentially removing that api mustache folder, that each serializer has to adapt.

  • for the first issue, we introduce 2 method signatures (raw + json, as described in my comment)
  • for the second issue, we introduce a new interface IJsonSerializationRepository which all serialization options have to implement.
    It will include these 2 abstract methods
    Object serialize<T>(T src, {Object? context});
    T deserialize<T>(Object value, {Object? context});
    where the general rule is that
    json == serialize<T>(deserialize<T>(json))
    object == deserialize<T>(serialize<T>(object))
@ahmednfwela
Copy link
Contributor Author

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

@kuhnroyal
Copy link
Contributor

for the first issue, we introduce 2 method signatures (raw + json, as described in my comment)

I don't like the idea of mixing the methods.
What if we generate a raw API instance, only depending on the HTTP framework dio|http but totally independent of any serialization library.
And then we use this inside our normal API classes.

class PetApi {

	final PetApiRaw rawApi;
	
	...
	
	Future<Response<Pet>> getPetById(String id, ...) async {
		// serialize ...
		final rawResponse = await rawApi.getPetById(id);
		/// deserialize ...
		return response;
	}
}

This would still allow consumers to construct/use/extent the raw API but it doesn't clutter the typed API classes.

@ahmednfwela
Copy link
Contributor Author

I am in favor of this as well, this will also minimize breaking changes

@ahmednfwela
Copy link
Contributor Author

@kuhnroyal please check this PR #15485

@kuhnroyal
Copy link
Contributor

@ahmednfwela I thought about this for a while, but had not time to thoroughly go through your PRs. But due to the limited amount of time left for 7.0.0, I think we should do this in a new dart-next experimental generator. The changes seem to big otherwise. WDYT?

@ahmednfwela
Copy link
Contributor Author

ahmednfwela commented Jun 15, 2023

@kuhnroyal I think this separation is critical right now, since maintaining the generator at its current state will be impossible once more features are added.
My PR was blocked because of #15830, which is now solved.
So I can resume working on this starting tomorrow, and if it's ok with @wing328 can we postpone the 7.0.0 release just a little bit ?

this PR was pretty much done except for the handling of container variable types

@kuhnroyal
Copy link
Contributor

I absolutely agree, I am just wondering if it is better to introduce a new experimental generator that can replace the old one in a later version. These changes are massive and I don't feel comfortable merging them into the current generator.

@ahmednfwela
Copy link
Contributor Author

@kuhnroyal The thing is, I was planning on merging both the dart-dio and dart into just dart.
so it will be already breaking for dart-dio users to migrate to the new dart generator.
and while a breaking change is annoying, its merits far outweigh the annoyances.

just look at the amount of issues dart and dart-dio currently have
most of them are easily fixable by unifying the codebase into serialization/api layers

@kuhnroyal
Copy link
Contributor

But there is no need to break 2 generators and replace them with one that is mostly untested when the release comes.
We can create a new generator and fix all the problems there, deprecate the old ones in a 7.1 release and then switch over in the next major release.

@ahmednfwela
Copy link
Contributor Author

ahmednfwela commented Jun 17, 2023

the main problem with that is:

  1. the new generator will either have to not extend AbstractDart generator or we will have to break it.
  2. the mustache files won't be in the same place as the previous 2 generators, since their structure changed
  3. I am not familiar with introducing new generators, so I would need some help with that

UPDATE: I have changed the PR to introduce a new generator instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants