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

✨ add support for top level variables #493

Merged
merged 2 commits into from
Aug 31, 2023

Conversation

martinale14
Copy link
Contributor

@martinale14 martinale14 commented Aug 29, 2023

I created this pr to give support to top level variables for the scenarios where we have multiple urls and that urls depend on ambient. for example we have our service A to make the main requests but we have service B to specific funcionality with diferent base url and maybe we want to change the service B to a dev version, so to change the url we have tu rebuild the chopper files because base url points to the literal value of the variable, with this change we don't need to rebuild in that scenerio because we can point the base url to a top level variable and that variable is going to be present in the generated file instead of the literal value

Imaging this case

const String enviromentUrl = Enviroments.temporal;

class Enviroments {
  static const String temporal = 'https://api.example.app';
}
@ChopperApi(baseUrl: enviromentUrl)
abstract class TeacherDatasource extends ChopperService {
  @Get(path: '/get_teachers/start/open')
  Future<Response<List<TeacherModel>>> getTeachers();

  static TeacherDatasource create([ChopperClient? client]) =>
      _$TeacherDatasource(client);
}

Before the change:

final class _$TeacherDatasource extends TeacherDatasource {
  _$TeacherDatasource([ChopperClient? client]) {
    if (client == null) return;
    this.client = client;
  }

  @override
  final definitionType = TeacherDatasource;

  @override
  Future<Response<List<TeacherModel>>> getTeachers() {
    final Uri $url =
        Uri.parse('https://api.example.app/get_teachers/start/open');
    final Request $request = Request(
      'GET',
      $url,
      client.baseUrl,
    );
    return client.send<List<TeacherModel>, TeacherModel>($request);
  }
}

After the change:

final class _$TeacherDatasource extends TeacherDatasource {
  _$TeacherDatasource([ChopperClient? client]) {
    if (client == null) return;
    this.client = client;
  }

  @override
  final definitionType = TeacherDatasource;

  @override
  Future<Response<List<TeacherModel>>> getTeachers() {
    final Uri $url = Uri.parse('${enviromentUrl}/get_teachers/start/open');
    final Request $request = Request(
      'GET',
      $url,
      client.baseUrl,
    );
    return client.send<List<TeacherModel>, TeacherModel>($request);
  }
}

@techouse
Copy link
Collaborator

Can you please elaborate on the change?

@codecov
Copy link

codecov bot commented Aug 29, 2023

Codecov Report

Merging #493 (da1c25b) into develop (c7b67a6) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #493   +/-   ##
========================================
  Coverage    93.83%   93.83%           
========================================
  Files            9        9           
  Lines          454      454           
========================================
  Hits           426      426           
  Misses          28       28           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@martinale14
Copy link
Contributor Author

Can you please elaborate on the change?

Thanks for the comment, I already edited the pr's comment

@techouse
Copy link
Collaborator

techouse commented Aug 29, 2023

for example we have our service A to make the main requests but we have service B to specific funcionality with diferent base url and maybe we want to change the service B to a dev version, so to change the url we have tu rebuild the chopper files because base url points to the literal value of the variable

Maybe I don't fully understand, but to me this solution sounds like a hack and I'm not sure that it's the responsibility of Chopper to give the user the option of having multiple environments.

Why not simply abstract all these baseUrls into multiple environment files and load that baseUrl into the ChopperClient, i.e.

final ChopperClient client = ChopperClient(
  baseUrl: prodEnv.apiUrl // <--
  // ... stuff
);

Sorry if I'm being too nosy, but can you maybe provide a few examples?

@techouse techouse self-assigned this Aug 29, 2023
@techouse techouse added the enhancement New feature or request label Aug 29, 2023
@martinale14
Copy link
Contributor Author

martinale14 commented Aug 29, 2023

for example we have our service A to make the main requests but we have service B to specific funcionality with diferent base url and maybe we want to change the service B to a dev version, so to change the url we have tu rebuild the chopper files because base url points to the literal value of the variable

Maybe I don't fully understand, but to me this solution sounds like a hack and I'm not sure that it's the responsibility of Chopper to give the user the option of having multiple environments.

Why not simply abstract all these baseUrls into multiple environment files and load those based on the desired scenario?

Sorry if I'm being too nosy, but can you maybe provide a few examples?

i updated the comment again with an example, no problem bro

@techouse
Copy link
Collaborator

i updated the comment again with an example, no problem bro

I've cleaned it up for you :)

I see what you mean. I'd usually solve this using a ChopperClient instance, however, this seems like a viable solution as well.

Right, can you add some tests for this, then I think we can merge it :)

@martinale14
Copy link
Contributor Author

i updated the comment again with an example, no problem bro

I've cleaned it up for you :)

I see what you mean. I'd usually solve this using a ChopperClient instance, however, this seems like a viable solution as well.

Right, can you add some tests for this, then I think we can merge it :)

ready man i upload some tests

Copy link
Collaborator

@techouse techouse left a comment

Choose a reason for hiding this comment

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

LGTM

@JEuler @Guldem your thoughts?

@Guldem
Copy link
Contributor

Guldem commented Aug 31, 2023

I don't think this addition doesn't do any harm. It doesn't directly change the way how some one would use chopper. Maybe a bit more documentation could be added in the doc files. So its not really a "hidden" feature.

Although I think I would have solved this by rewriting baseUrl in a Interceptor. If I had to change thinks runtime.

@techouse techouse merged commit e706150 into lejard-h:develop Aug 31, 2023
6 checks passed
techouse added a commit that referenced this pull request Sep 1, 2023
# chopper

## 7.0.6

- #493
- #495

# chopper_built_value

## 2.0.1

- #495

# chopper_generator

## 7.0.3

- #493
- #495
@techouse techouse mentioned this pull request Sep 1, 2023
@techouse techouse changed the title Adding support for top level variables ✨ add support for top level variables Sep 2, 2023
techouse added a commit that referenced this pull request Sep 2, 2023
# chopper

## 7.0.6

- #493
- #497
- #495

# chopper_built_value

## 2.0.1

- #495

# chopper_generator

## 7.0.3

- #493
- #495

---------

Co-authored-by: Klemen Tusar <techouse@gmail.com>
Co-authored-by: Martin Alejandro Escobar Espinel <56127727+martinale14@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants