Add Node build for Firebase Storage #5468
Conversation
|
schmidt-sebastian
force-pushed
the
mrschmidt/nodebuild
branch
from
1a183d5 to
36eb98a
Compare
Changeset File Check ✅
|
This adds a Node and a Browser specific build so we can have getStream() in Node and getBlob() in browsers
schmidt-sebastian
force-pushed
the
mrschmidt/nodebuild
branch
from
36eb98a to
00cfe38
Compare
packages/storage/src/api.node.ts
Outdated
| * limitations under the License. | ||
| */ | ||
|
|
||
| // TODO(mrschmidt): Add getStream() |
There was a problem hiding this comment.
Not a big fan of having different API for browser and node.
It makes it harder for developers to write isomorphic code for use cases like SSR. It also makes documentation harder, as we need to document the differences. We will also need to provide 2 typing files.
What does getStream do? Is it possible to make it part of getBlob with a flag?
There was a problem hiding this comment.
I don't think we can combine getBlob and getStream. The Blob API is not available in Node, and Streams are not available for Browsers.
I have two suggestions:
- Remove getStream()
- Export getBytes()/getBlob() for all platforms, but make it throw for platforms that are not supported.
I updated the PR to throw.
There was a problem hiding this comment.
Okay. I agree with throwing and keeping the same API for browser and node. We have taken the same approach in Auth to keep their node and browser API consistent.
Binary Size ReportAffected SDKsTest Logs |
Size Analysis Report |
| * retrieve. | ||
| * @returns A Promise that resolves with a Blob containing the object's bytes | ||
| */ | ||
| function getBlob( |
There was a problem hiding this comment.
Should it be exported here and in api.node.ts?
There was a problem hiding this comment.
That would immediately add it to the public API. Let me suppress the warning for now.
This adds a Node and a Browser specific build so we can have getStream() in Node and getBlob() in browsers
This reduces the diff for #4973.