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

Avoid calling getDownloadURL in favor of directly getting data #241

Closed
wants to merge 11 commits into from
Closed

Avoid calling getDownloadURL in favor of directly getting data #241

wants to merge 11 commits into from

Conversation

stargazing-dino
Copy link

@stargazing-dino stargazing-dino commented Oct 21, 2020

✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)

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

💥 Does this PR introduce a breaking change?

No

📝 Links to relevant issues/docs

firebase/firebase-js-sdk#76

🤔 Checklist before submitting

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

@stargazing-dino stargazing-dino changed the title Avoid calling getDownloadURL in favor of directly getting data from Avoid calling getDownloadURL in favor of directly getting data Oct 21, 2020
@jdve
Copy link

jdve commented Oct 21, 2020

Thanks for this PR! I hit an error due to CacheControl being null. I fixed it with this follow on change:

steppinglight@e0463d0

@stargazing-dino
Copy link
Author

Thank you! I added your changes. I'll have to finish this on the weekend as another project is taking up my time

@stargazing-dino
Copy link
Author

I just recently checked this again and it looks like it's working for me as expected. It might've been a cached older package I was using by accident. I'll move this out of draft and work on checklist. Pleas feel free to offer objections to my alternative

@stargazing-dino stargazing-dino marked this pull request as ready for review November 4, 2020 02:06
@stargazing-dino
Copy link
Author

I'm afraid this is too stale. Closing for now to allow NNBD PR prior to any action here

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