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

ThreadSafetyTests: add failing test where we load and store an image #755

Merged
merged 2 commits into from
Mar 3, 2024
Merged

ThreadSafetyTests: add failing test where we load and store an image #755

merged 2 commits into from
Mar 3, 2024

Conversation

AndrewSB
Copy link
Contributor

adds a test that shows #750

@kean
Copy link
Owner

kean commented Feb 24, 2024

Hey @AndrewSB, thanks for providing the sample code. I now realized what I was missing, and it was pretty obvious.

The default pipeline (ImagePipeline.shared) does not use Nuke.DataCache but instead relies on the Foundation.URLCache. Unfortunately, the following APIs only with the custom caches added by Nuke and not the caches that are part of the data loading system:

ImagePipeline.shared.cache.storeCachedData(imageData, for: ImageRequest(url: remoteURL)

In order to make this test pass, you need to do the following:

// 1. Enable custom disk cache
let pipeline = ImagePipeline(configuration: .withDataCache)

// 2. Use real image data, otherwise the cached data gets ignored
let imageData = try XCTUnwrap(Test.data(name: "fixture", extension: "jpeg"))

With these two changes, you get the following response:

    ▿ cacheType : Optional<CacheType>
      - some : Nuke.ImageResponse.CacheType.disk

Caching is one of the more compiled aspects in any image loading framework, so I would recommend going through the related documentation.

@kean kean merged commit 3b4c97f into kean:main Mar 3, 2024
8 checks passed
@kean
Copy link
Owner

kean commented Mar 3, 2024

Thanks for updating the test. I'm going to merge and update it a bit – it shouldn't use the default cache.

@AndrewSB
Copy link
Contributor Author

AndrewSB commented Mar 5, 2024

that makes sense! i wrote it that way because that's how i'm using it in my project, i assumed you'd want on which is created inline. appreciate the help @kean!

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.

2 participants