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

Added the 'expose_http_client' flag #453

Closed

Conversation

jeroenvervaeke
Copy link
Contributor

I want to be able to supply a http client to the generated code. This is useful for configuring things like proxies but also for unit testing.

I've added a build option expose_http_client that you can add in the build.yaml.

With the flag set to false (default), the code remains exactly as it was before adding this feature.
When it's enabled the following is generate:

  static Api create(
      {ChopperClient? client,
      Authenticator? authenticator,
      String? baseUrl,
      Iterable<dynamic>? interceptors,
      http.Client? httpClient}) {
    if (client != null) {
      return _$Api(client);
    }

    final newClient = ChopperClient(
        services: [_$Api()],
        converter: $JsonSerializableConverter(),
        interceptors: interceptors ?? [],
        authenticator: authenticator,
        client: httpClient,
        baseUrl: baseUrl ?? 'http://');
    return _$Api(newClient);
  }

The reason why I put this behind a flag is because you need a dependency to http when you want to accept http.Client as a parameter, which would be a breaking change.

@codecov
Copy link

codecov bot commented Aug 24, 2022

Codecov Report

Merging #453 (37be963) into master (da9ea15) will decrease coverage by 0.03%.
The diff coverage is 94.87%.

@@            Coverage Diff             @@
##           master     #453      +/-   ##
==========================================
- Coverage   75.51%   75.48%   -0.03%     
==========================================
  Files           9        9              
  Lines         964      971       +7     
==========================================
+ Hits          728      733       +5     
- Misses        236      238       +2     
Impacted Files Coverage Δ
lib/src/models/generator_options.dart 25.00% <ø> (ø)
lib/src/models/generator_options.g2.dart 0.00% <0.00%> (ø)
...c/code_generators/swagger_additions_generator.dart 79.48% <100.00%> (+0.53%) ⬆️
...rc/code_generators/swagger_requests_generator.dart 92.72% <100.00%> (+0.05%) ⬆️

@fryette
Copy link
Contributor

fryette commented Aug 26, 2022

You can create ChopperClient and provide to it custom httpClient with all proxies.
ChopperClient it's simple wrapper.

So my question is why you cannot send configured ChopperClient(with HttpClient inside) instead of HttpClient?

@jeroenvervaeke
Copy link
Contributor Author

So my question is why you cannot send configured ChopperClient(with HttpClient inside) instead of HttpClient?

To make the library easier to use. Having to create the ChopperClient yourself kind of defeats the purpose of having an easy to use automatically generated client.

One could argue the same about authenticator, baseUrl and the interceptors. Why don't drop these arguments and just pass the chopper client there?

static PetServiceJson create(
      {ChopperClient? client,
      Authenticator? authenticator,
      String? baseUrl,
      Iterable<dynamic>? interceptors}) {
    if (client != null) {
      return _$PetServiceJson(client);
    }

    final newClient = ChopperClient(
        services: [_$PetServiceJson()],
        converter: $JsonSerializableConverter(),
        interceptors: interceptors ?? [],
        authenticator: authenticator,
        baseUrl: baseUrl ?? 'http://petstore.swagger.io/v2');
    return _$PetServiceJson(newClient);
  }

source: https://github.com/epam-cross-platform-lab/swagger-dart-code-generator/blob/master/example/lib/swagger_generated_code/pet_service_json.swagger.dart#L23

Optionally adding the HttpClient to the create factory is consistent with how the other parameters are configured ( authenticator, baseUrl, interceptors).

The way I've added it also makes it entirely optional, so it doesn't cause anything to be backward incompatible.

@Vovanella95
Copy link
Collaborator

I'm closing the PR because you can expose ChopperClient even without these changes.

@Vovanella95 Vovanella95 closed this Sep 7, 2022
@Dumoulin-Lander
Copy link

@Vovanella95 Can you elaborate on how it's possible to expose the ChopperClient?

@Vovanella95
Copy link
Collaborator

Hi @Dumoulin-Lander , you can do this when create service instance:

  final newClient = ChopperClient(
    converter: $JsonSerializableConverter(),
    //any parameters you want
  );

  final service = PetServiceJson.create(client: newClient);

@Dumoulin-Lander
Copy link

Thanks for the quick response!

I find myself agreeing with Jeroen here I think. The way you're proposing gives you ways to retain a reference to the ChopperClient you pass in, but it does not expose the client out from the service instance.

I guess the ship has sailed on this PR, I'll figure something out myself. Thanks again!

@jeroenvervaeke
Copy link
Contributor Author

Feel free to use my forked repository, I'm don't plan on removing it since I'm using this fork in some of my other projects.

@Dumoulin-Lander
Copy link

Feel free to use my forked repository, I'm don't plan on removing it since I'm using this fork in some of my other projects.

There's already an issue fixed in this main repository that is valuable for me, so I'd rather use it so we can get updates when needed.

Also, hi Jeroen :)

@Vovanella95
Copy link
Collaborator

@Dumoulin-Lander if it's really important, ok we can merge it :) I am reopening PR, please ensure CI proccess is succeed

@Vovanella95 Vovanella95 reopened this Jun 15, 2023
@Vovanella95
Copy link
Collaborator

@Dumoulin-Lander please resolve conflicts

@Vovanella95
Copy link
Collaborator

We can expose ChopperClient without this PR. Closing it.

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.

4 participants