feat(firebase_storage_web): web support #3917
Conversation
| /// Upload a [Blob]. Note; this is only supported on web platforms. | ||
| /// | ||
| /// Optionally, you can also set metadata onto the uploaded object. | ||
| TaskPlatform putBlob(dynamic data, [SettableMetadata metadata]) { |
There was a problem hiding this comment.
Any ideas what we can do here @ditman for actually having a Blob type rather than dynamic? I'm not sure how it works on the public facing library since we would need to be pulled in. I was thinking of a stub class, which web overrides?
There was a problem hiding this comment.
Hm, maybe an assert(data is html.Blob), or some other runtime check?
We can't really import anything from dart:html when compiling for mobile (the library is not available there); do you think we should provide a BlobPlatform type? What would that do in the native layer, just wrap the bytes?
|
I ran some e2e tests in package:firebase_storage for web, and got the following (very useful) report: Driver tests failed: 15 (-d chrome):I'll try to get as many tests fixed as possible (things dealing with "File" won't work, obviously), but there are some test cases there for error handling that are very handy! |
|
I just realized that I'm using the wrong project credentials (my own), so OF COURSE some of the expected/unexpected files are not present; I'll try to reproduce the expected layout in my own firebase storage. |
You could copy the web credentials from the Core, Auth or Firestore example - should work for existing tests then :) |
🤦♂️ |
|
I fixed most of the tests, there's only two outstanding issues:
Driver tests failed: 3 (-d chrome):Most of the changes I did in tests were to "skip" tests that are not supposed to run in Web (I fixed one that was skipping !kIsWeb when it should not be doing that). Some less legit test changes:
|
|
Finally got the "expected a FirebaseException" tests sorted! The last couple of them were caused because I overlooked how the frontend Task connected its implementation of the Future interface to the underlying delegate task; I missed that I needed to handle errors in the JS SDK task.future, and not only in its onComplete Stream. The patch above fixes that issue. Driver tests failed: 1 (-d chrome):The latest test remaining will need some extra logic to be implemented in the web side; I need to "remember" on each ref what metadata has been set, so I can re-apply it upon further calls to updateMetadata. This is much easier on JS (only updateMetadata with the settings that you intend to set), but that API is lost with our SettableMetadata definition (and the JS interop's, it seems). I'll patch it in the reference_web directly tomorrow, and then get some unit tests for the web package as well! I think this is starting to be a fairly usable implementation of the storage package for web! |
|
All e2e tests seem to be passing now! |
|
I'll try to add some unit tests, but since |
ditman
changed the title
This helper implements the logic of how settable metadata works (set once per ref).
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
@googlebot I consent. |
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
@googlebot I consent. |
Salakar
changed the title
|
🎉 Congratulations for the merge of this PR @ditman . |
|
@jesusrp98 what's your pubspec.yaml? I think you need to override both firebase_storage and web from git to grab the latest code. I don't think _web has been published to pub.dev yet. |
|
@ditman No worries, it has been published to pub.dev! firebase_storage v5.1.0 :) |
Description
This PR introduces a web version for firebase_storage. It is unit tested.
This package has been integration-tested against the current integration tests of firebase_storage. Some tweaks were needed for the tests, but those are in a separate PR, so they can be merged separately.
Related Issues
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]).This will ensure a smooth and quick review process. Updating the
pubspec.yamland changelogs is not required.///).flutter analyze) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?