Skip to content

Avoid calling getDownloadURL in favor of directly getting data #322

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

Closed
wants to merge 19 commits into from
Closed

Avoid calling getDownloadURL in favor of directly getting data #322

wants to merge 19 commits into from

Conversation

stargazing-dino
Copy link

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

✨ 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

⤵️ 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)

🆕 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.

💥 Does this PR introduce a breaking change?

No

🐛 Recommendations for testing

📝 Links to relevant issues/docs

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

🤔 Checklist before submitting

  • All projects build
  • Follows style guide lines (code style guide)
  • Relevant documentation was updated
  • Rebased onto current develop

@stargazing-dino
Copy link
Author

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();
  });
}

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.

4 participants