Baseflow / flutter_cache_manager

Generic cache manager for flutter
https://baseflow.com
MIT License
739 stars 426 forks source link

Avoid calling getDownloadURL in favor of directly getting data #322

Closed stargazing-dino closed 2 years ago

stargazing-dino commented 3 years ago

Note I accidentally deleted my old repo and couldn't reopen my previous closed PR so this is just a reopening of #241

:sparkles: What kind of change does this PR introduce? Enhancement

This would handle #240. I'm scared this adds an extra network call (possibly two but I don't think getReferenceFromUrl is a network req) so I won't I expect anything from this PR and the simplicity of getDownloadURL might be preferred.

I think the original bug is that the the extension is not being carried through to the path in either my implementation or the current one and that's what's causing this line to fail in the original issue:

final file = await FirebaseCacheManager().getSingleFile(audioURL);
// returns a file with a path of local_path/foo_bar instead of local_path/foo_bar.mp3

:arrow_heading_down: What is the current behavior?

The current behavior is to call getDownloadUrl which creates a long lived URL. This is great for users that need the url but for users who are just trying to show an image or file one time, the creation of the url seems unneeded. (Also, I just checked and it looks the current FirebaseCacheManager is not even working for me. Getiing "file not found at that location" or something)

:new: What is the new behavior (if this is a feature change)?

It creates a ref and then directly downloads the data. The headers are created from a call to getMetaData and are converted to http headers to match.

:boom: Does this PR introduce a breaking change?

No

:bug: Recommendations for testing

:memo: Links to relevant issues/docs

firebase/firebase-js-sdk#76 https://www.sentinelstand.com/article/guide-to-firebase-storage-download-urls-tokens

:thinking: Checklist before submitting

stargazing-dino commented 3 years ago

Not sure how to get around this but I tried to create a test for this and I couldn't because it failed here

static Future<Directory> createDirectory(String key) async {
    var baseDir = await getTemporaryDirectory(); // <--
    var path = p.join(baseDir.path, key);

    var fs = const LocalFileSystem();
    var directory = fs.directory((path));
    await directory.create(recursive: true);
    return directory;
  }

I think it's because it can't get a temp directory in a test environment ? It's looking for method channels which I don't think would exist. Anyone know how to get around this?

Here's the test code:

import 'package:firebase_storage_mocks/firebase_storage_mocks.dart';

final filename = 'someimage.png';

void main() {
  test('Gets data from a storage ref', () async {
    final storage = MockFirebaseStorage();
    final storageRef = storage.ref().child(filename);
    final image = File(filename);
    final task = storageRef.putFile(image);
    await task.whenComplete(() {});
    final cacheManager = FirebaseCacheManager();

    cacheManager.downloadFile(storageRef.fullPath);

    storageRef.getData();
  });
}