hurshi / dio-http-cache

http cache lib for Flutter dio like RxCache
Apache License 2.0
274 stars 223 forks source link

Migrate to nullsatefy and Dio 4 #84

Closed erickok closed 3 years ago

erickok commented 3 years ago

Migrates to Dart 2.12 and enabled nullsafety. This also upgrades to Dio 4's new interceptors with handlers.

The example was updated for Android to use v3 embedding. Updated iOS example app Podfile.

--

Similar to #81 but compatible with Dio 4.

alr2413 commented 3 years ago

ping @hurshi. please merge it if there is no issue

slavap commented 3 years ago

@erickok Please make DioCacheManager._buildResponse() non-private and async (make it protected), with changed to RequestInterceptorHandler it is currently too hard to adjust cached data additionally in DioCacheManager descendants.

@protected
Future<Response> buildResponse(CacheObj obj, int statusCode, RequestOptions options) async {
}

And add await in two places of DioCacheManager where buildResponse() called.

erickok commented 3 years ago

@slavap I am not the maintainer of the library so I am not sure I want to introduce such changes in this PR. Maybe you can submit a separate PR instead (branched from this nullsafety PR)?

Otherwise if you explain a bit more what you want to do, maybe there is a different way - the new handler-bsed interceptors are exactly build with a use-case in mind where you want to amend the default behaviour.

slavap commented 3 years ago

@erickok it is proper change for new handler based structure. Example: there are some data in cache and they are still valid, but some headers and cookies are changed, so I need just slightly adjust Response without discarding the cache. So I have descendant which manages this after data returned from the cache.

erickok commented 3 years ago

@slavap Fair enough but in the end it's not really my decision to make.

slavap commented 3 years ago

@erickok As you see author of this repo is very slowly reacting. My change is absolutely minor and does not affect anything in existing codebase. But fits properly into new dio logic.

erickok commented 3 years ago

@slavap I understand. Especially because the owner is so slow, I suggest you fork the library yourself, apply this pr and your preferred change, and just use that as ref in your flutter project dependencies. I don't think i can be of much help to you.

slavap commented 3 years ago

@erickok ok, thanks. Of course I’m using my own version of this lib, but it is plainly wrong.

mozaffari commented 3 years ago

Hey maintainer, merge this as soon as possible. I had to download @erickok 's version and use it as local.

 dio_http_cache: 
   path: "./local_packages/dio-http-cache-feature-nullsafety-dio4"
aldychris commented 3 years ago

Hey maintainer, merge this as soon as possible. I had to download @erickok 's version and use it as local.

dio_http_cache: path: "./local_packages/dio-http-cache-feature-nullsafety-dio4"

I do this while waiting @hurshi to review and merge it

dio_http_cache: 
    git:
      url: https://github.com/vrtdev/dio-http-cache.git
      ref: feature/nullsafety-dio4
mozaffari commented 3 years ago

Hey maintainer, merge this as soon as possible. I had to download @erickok 's version and use it as local. dio_http_cache: path: "./local_packages/dio-http-cache-feature-nullsafety-dio4"

I do this while waiting @hurshi to review and merge it

dio_http_cache: 
    git:
      url: https://github.com/vrtdev/dio-http-cache.git
      ref: feature/nullsafety-dio4

Thanks, this is way better. I will use this syntax

tneotia commented 3 years ago

Hi @erickok is there a reason you made baseUrl required? As per the docs it doesn't need to be required and I want to make sure that omitting it in my production app doesn't cause any issues.

erickok commented 3 years ago

Hmm you might be right. I though it was a strict requirement (as there were non-null azssertions in _getUriByPath) but reading through it again indeed it allows not setting a baseUrl if the full path is provided when deleting from cache. I will update this.

erickok commented 3 years ago

Fixed - thanks for the feedback.

franticn commented 3 years ago

@erickok Thank you very much for your PR. We will support nullsafety and Flutter 2.0 through new branches in the future, but before that, we need to do some cache features and bug fixes.

slavap commented 3 years ago

@franticn Please also add changes described here: https://github.com/hurshi/dio-http-cache/issues/85 after merging this branch,

renntbenrennt commented 3 years ago

@slavap hey, is your change going to break the original stuff? Or is it necessary to add/change? Otherwise I will just make the dep link to @erickok 's fork version until this package is update, which I hope it will be out before the climate change take over the civilization...

Ps, I don't directly use the package, just the package I have to use use this... and that package is outdated as well... and the updating work lead me to here... PPs, there's a holiday in China coming up, but you guys have to be aware that the working situation in China is not heaven at all, with those 996 life style, I kind of surprised by the fact that this package is not one of kpi product, so kudos to the owner and pray that he has time back to his stuff instead of draining by the work(assuming he is occupied by the work...)

slavap commented 3 years ago

@SeasonLeee

  1. No, won't break anything.
  2. It is nice to have change considering Dio 4 changes, it will seriously simplify implementation of descendants.
renntbenrennt commented 3 years ago

@slavap yeah... sure... thanks... but I'm not the user of this package nor that dio, so no comment on here😅

It just one of the package I need to use depend on this.... 😩... either way... I'm just going to use @erickok 's fork... because I got job to finish😄

mozaffari commented 3 years ago

@erickok you have to handler.resolve(response); _onError if maxStale is set.

Otherwise, neither error happens or the request completes if maxStale is not expired and there is a prev success request.

this happens when there is an error and the cache age is expired but maxStale is set to some future date so inside _onRequest _pullFromCacheBeforeMaxAge(options) is null and if (null != responseDataFromCache) is false, so _onRequest can not resolve the response.

now _onError the _pullFromCacheBeforeMaxStale(e.requestOptions); can return a previous success response because there is an old cached request available but inside of if (null != responseDataFromCache) {...} block of code instead of resolving the response we return the response directly and that can not resolve the response and handler.next(e) also can not be called because to the return _buildResponse(responseDataFromCache, responseDataFromCache.statusCode, e.requestOptions);

Bellow is my full _onError method version of your PR

  _onError(DioError e, ErrorInterceptorHandler handler) async {
    if ((e.requestOptions.extra[DIO_CACHE_KEY_TRY_CACHE] ?? false) == true) {
      var responseDataFromCache =
          await _pullFromCacheBeforeMaxStale(e.requestOptions);
      if (null != responseDataFromCache) {
        var response = _buildResponse(responseDataFromCache,
            responseDataFromCache.statusCode, e.requestOptions);

        return handler.resolve(response);
      }
    }
    return handler.next(e);
  }
erickok commented 3 years ago

@mozaffari Thank you for finding and reporting this! I updated my code

franticn commented 3 years ago

@erickok @mozaffari Thanks for your pr.

haroonkhan9426 commented 3 years ago

Following the same issue.