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

Change return type of decodeJson to FutureOr in order to be able to support compute() #345

Merged
merged 5 commits into from
Sep 7, 2022

Conversation

techouse
Copy link
Collaborator

@techouse techouse commented Jul 12, 2022

This partially addresses #158 and is mostly based on @lejard-h's suggestion #158 (comment)

This is a breaking change as it changes the response types of some JsonConverter methods, namely:

  • decodeJson now returns FutureOr<Response>

    /// before
    Response decodeJson<BodyType, InnerType>(Response response) {
      /* sync decode stuf */
    }
    
    /// after
    FutureOr<Response> decodeJson<BodyType, InnerType>(Response response) async {
      /* async decode stuff */
    }
  • convertResponse now returns FutureOr<Response<BodyType>>

    /// before
    @override
    Response<BodyType> convertResponse<BodyType, InnerType>(Response response) {
      /* sync convert stuff */
    }
    
    /// after
    @override
    FutureOr<Response<BodyType>> convertResponse<BodyType, InnerType>(Response response) async {
      /* async convert stuff */
    }
  • tryDecodeJson now returns FutureOr<dynamic> and is now public

    /// before
    dynamic _tryDecodeJson(String data) {
      /* decode json */
    }
    
    /// after
    FutureOr<dynamic> tryDecodeJson(String data) {
      /* now you can override this method and use compute to decode json */
    }
  • convertError now returns FutureOr<Response>

    /// before
    @override
    Response convertError<BodyType, InnerType>(Response response) {
      /* sync decode error */
    }
    
    /// after
    @override
    FutureOr<Response> convertError<BodyType, InnerType>(Response response) async {
      /* async decode error */
    }
  • responseFactory now returns FutureOr<Response<BodyType>>

    /// before
    static Response<BodyType> responseFactory<BodyType, InnerType>(Response response) {
      /* stuff */
    }
    
    /// after
    static FutureOr<Response<BodyType>> responseFactory<BodyType, InnerType>(Response response){
      /* stuff */
    }

@techouse techouse marked this pull request as ready for review July 12, 2022 10:23
@techouse
Copy link
Collaborator Author

This PR most likely needs further scrutiny as it is breaking.

@vaetas
Copy link

vaetas commented Jul 14, 2022

Is there a reason to use FutureOr instead of Future in this context? I somehow understand the difference between the two. I am not sure about the technical implications here. I am very curious though.

The API request itself must be Future. Does it therefore matter that inner methods are FutureOr?

@techouse
Copy link
Collaborator Author

techouse commented Jul 14, 2022

Is there a reason to use FutureOr instead of Future in this context? I somehow understand the difference between the two. I am not sure about the technical implications here. I am very curious though.

The API request itself must be Future. Does it therefore matter that inner methods are FutureOr?

Both convertRequest and converResponse of the parent Converter are FutureOr.

On top of that one can override a FutureOr to be sync, I.e. FutureOr<Response> can be overridden just as Response (what it currently is in the stable release).

@techouse techouse mentioned this pull request Jul 15, 2022
@techouse
Copy link
Collaborator Author

techouse commented Jul 23, 2022

I had to pin analyzer to >=4.1.0 <4.3.0 because v4.3.0 deprecates Element.enclosingElement and advises to use Element.enclosingElement2 which is not present in v4.2.0 and below.

It's more or less an issue with the Github Action because it doesn't use lock files but rather upgrades to the latest dependencies.

@JEuler
Copy link
Collaborator

JEuler commented Aug 13, 2022

Hm, I'm returning to this thing. As it is breaking we will need to double think it. Personally, I think we can go with your both PR's.
@stewemetal What do you think? :)

@JEuler
Copy link
Collaborator

JEuler commented Sep 5, 2022

So, I will merge both of your PRs and create a new major update of the Chopper. Thank you for the great work. I think I will do this tomorrow!

@JEuler JEuler merged commit f9009ce into lejard-h:develop Sep 7, 2022
@techouse techouse deleted the futureor-json-converter branch September 7, 2022 09:42
JEuler pushed a commit that referenced this pull request Oct 8, 2022
JEuler added a commit that referenced this pull request Oct 15, 2022
* Fix Header Option Casting (#260)

Co-authored-by: Ivan Terekhin <i.terhin@gmail.com>

* Fix for #259 (#263)

* 4.0.1 fixes (#264)

* analyzer dependency upgraded (#296)

* fix(generator): fix PartValueFile value not nullable if arg is (#288) (#293)

* Chopper generator release 4.0.2 (#297)

* fix: fix this.body cast of null value when response body is null (#291) (#292)

* Interpolation fixes (#275)

* encodeQueryComponent now encodeComponent (#278)

* Prevent double call on token refreshment (#276)

* Fixes for #309 #308 (#310)

* Remove new keyword from interceptors.md (#312)

* Analyzer upgrade (#320)

Co-authored-by: István Juhos <stewemetal@gmail.com>

* Add unnecessary_brace_in_string_interps to lint ignores (#317)

* Extend pragma to quiet the linter (#318)

Co-authored-by: Ivan Terekhin <i.terhin@gmail.com>

* Fix converter getting called twice if using an authenticator with a JsonConverter on the request (#324)

* migrate example to nullsafety (#331)

* Resolve problem in main_json_serializable example (#328)

* Add @FiledMap @PartMap @PartFileMap (#335)

Co-authored-by: Meysam Karimi <mysmartapply.it4@gmail.com>

* Upgrade of analyzer (#340)

* Fix nullable QueryMap fails to compile (#344)

* Change return type of decodeJson to FutureOr in order to be able to support compute() (#345)

* Migrate from pedantic to lints ^2.0.0 with lints/recommended.yaml (#349)

* Version bumped for release (#352)

* Revert analyzer to ^4.1.0 and silence linters for Element.enclosingElement (#354)

* [chopper_generator] Update analyzer to ^4.4.0 and code_builde to ^4.3.0 and migrate deprecated code (#358)

* Add Makefiles to streamline development (#357)

* Add Bug Report Github issue template (#359)

* [chopper_generator] Add types to the generated variables (#360)

* Provide an example using an Isolate Worker Pool with Squadron (#361)

* mapToQuery changes (#364)

* Version bumped / changelog update (#367)

* Request extends http.BaseRequest (#370)

* Exclude null query vars by default and add new @method annotation includeNullQueryVars (#372)

* 5.1.0 (dev) (#373)

Co-authored-by: Ivan Terekhin <231950+JEuler@users.noreply.github.com>

Co-authored-by: Youssef Raafat <youssefraafatnasry@gmail.com>
Co-authored-by: luis901101 <luis901101@gmail.com>
Co-authored-by: melvspace <ratealt@gmail.com>
Co-authored-by: Michal Šrůtek <35694712+michalsrutek@users.noreply.github.com>
Co-authored-by: István Juhos <stewemetal@gmail.com>
Co-authored-by: Andre <andre.lipke@gmail.com>
Co-authored-by: John Wimer <john@wimer.org>
Co-authored-by: Max Röhrl <max.roehrl11@gmail.com>
Co-authored-by: ipcjs <gipcjs@gmail.com>
Co-authored-by: ibadin <exbatek@gmail.com>
Co-authored-by: Meysam Karimi <31154534+meysam1717@users.noreply.github.com>
Co-authored-by: Meysam Karimi <mysmartapply.it4@gmail.com>
Co-authored-by: Klemen Tusar <techouse@gmail.com>
Co-authored-by: Klemen Tusar <k.tusar@cmcmarkets.com>
Co-authored-by: Ivan Terekhin <231950+JEuler@users.noreply.github.com>
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.

3 participants