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

Make DioCacheManager._buildResponse() non-private #85

Open
slavap opened this issue Mar 25, 2021 · 4 comments · May be fixed by #92
Open

Make DioCacheManager._buildResponse() non-private #85

slavap opened this issue Mar 25, 2021 · 4 comments · May be fixed by #92

Comments

@slavap
Copy link

slavap commented Mar 25, 2021

@hurshi
After accepting #84

Please add the following minor changes:

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.

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.

@franticn
Copy link
Collaborator

franticn commented Apr 2, 2021

Thanks for your commit, I will review this change on a non-working day, please be patient.

@franticn
Copy link
Collaborator

franticn commented Apr 4, 2021

@slavap After reviewing your change, I found that we need to upgrade to Dio 4 to support these, so the feature for support flutter 2.0 and null-safety maybe need to be advanced in advance.

@slavap
Copy link
Author

slavap commented Apr 4, 2021

@franticn yes, that’s what I said in the opening description, Dio 4 and null safety PR must be accepted first.

@slavap
Copy link
Author

slavap commented Apr 22, 2021

@franticn Please add this change, needed PR in already merged.

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 a pull request may close this issue.

2 participants