nalexn / clean-architecture-swiftui

SwiftUI sample app using Clean Architecture. Examples of working with CoreData persistence, networking, dependency injection, unit testing, and more.
MIT License
5.74k stars 698 forks source link

Add simple caching on NSCache #27

Closed ascherbinin closed 4 years ago

ascherbinin commented 4 years ago

Hi Alexander, thanks for your work.

I noticed a strange behavior here: when I exit the modal window, the flag picture reloaded every time, which doesn’t look beautiful. And I decided to try adding image caching to your implementation. It turned out quite primitive, but it works. If you like it, you can add.

Although in reality most likely you need to solve another problem so that the View with the flag does not restart after exiting the modal window, since it is already on the stack, but it is most likely much more complicated.

Sincerely, Andrey Shcherbinin.

codecov-io commented 4 years ago

Codecov Report

Merging #27 into mvvm will decrease coverage by 0.02%. The diff coverage is 92.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##             mvvm      #27      +/-   ##
==========================================
- Coverage   97.49%   97.47%   -0.03%     
==========================================
  Files          77       77              
  Lines        3319     3328       +9     
==========================================
+ Hits         3236     3244       +8     
- Misses         83       84       +1     
Impacted Files Coverage Δ
CountriesSwiftUI/Services/ImagesService.swift 95.00% <91.66%> (-5.00%) :arrow_down:
CountriesSwiftUI/System/AppDelegate.swift 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1957eb5...06001a3. Read the comment docs.

nalexn commented 4 years ago

Hey Andrey,

Thank you for your contribution! The project used to have such image caching, I've decided to remove it in favor of the standard caching at the networking level.

The main reason for this was that I want this project to collect as little re-invented wheels as possible, keeping the example simple. And this project is an example in the first place. Pretty UI/UX is secondary in this regard.

In a real-world app, the primary issue you referred to should not be so apparent. In this app, the images are returned as SVGs, so the app has to make 3 requests instead of just 1 for downloading them. The requirement to pull 3 cached responses in a row is what's causing the loading indicator to appear for longer than it should, but I agree with you that the app should not reload in the first place. I haven't investigated why this is happening.

There is a ticket that outlines an edge case where the standard caching can break, so I'm going to improve the caching to address that problem, but it'll still be caching at the networking layer.

I'm closing this pull request without merging, but feel free to help me with that ticket!