Skip to content

Add getBytes() #5672

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

Merged
merged 28 commits into from
Nov 11, 2021
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
62eadc3
Add getBytes()
schmidt-sebastian Oct 27, 2021
bbe754a
Create clever-eggs-relate.md
schmidt-sebastian Oct 27, 2021
241e1ff
Update clever-eggs-relate.md
schmidt-sebastian Oct 27, 2021
9ad21e0
Update API reports
schmidt-sebastian Oct 27, 2021
d377aa7
Small fixes
schmidt-sebastian Oct 27, 2021
1d7ba87
Merge branch 'mrschmidt/getbytes' of github.com:firebase/firebase-js-…
schmidt-sebastian Oct 27, 2021
9d42583
Browser error handling
schmidt-sebastian Oct 27, 2021
b300fe9
Update connection.ts
schmidt-sebastian Nov 7, 2021
718c537
Update connection.ts
schmidt-sebastian Nov 7, 2021
9838a74
Update packages/storage/src/platform/connection.ts
schmidt-sebastian Nov 9, 2021
bbfba22
Update packages/storage/src/platform/connection.ts
schmidt-sebastian Nov 9, 2021
e4d2cc9
Update packages/storage/src/platform/connection.ts
schmidt-sebastian Nov 9, 2021
3396393
Update packages/storage/src/platform/node/connection.ts
schmidt-sebastian Nov 9, 2021
8fef258
Update clever-eggs-relate.md
schmidt-sebastian Nov 9, 2021
ce4b617
Review
schmidt-sebastian Nov 9, 2021
c8bc1ac
Merge branch 'mrschmidt/getbytes' of github.com:firebase/firebase-js-…
schmidt-sebastian Nov 9, 2021
32e8978
Merge branch 'master' into mrschmidt/getbytes
schmidt-sebastian Nov 9, 2021
f5f71e5
Update .changeset/clever-eggs-relate.md
schmidt-sebastian Nov 9, 2021
fce32fe
Undo
schmidt-sebastian Nov 9, 2021
b0a2c6c
Cleanup
schmidt-sebastian Nov 9, 2021
8fb010b
Merge branch 'mrschmidt/getbytes' of github.com:firebase/firebase-js-…
schmidt-sebastian Nov 9, 2021
850a6a5
Simplify
schmidt-sebastian Nov 9, 2021
7637d20
Update API reports
schmidt-sebastian Nov 9, 2021
7955731
Update requests.ts
schmidt-sebastian Nov 10, 2021
3aa8b21
Cleanup
schmidt-sebastian Nov 10, 2021
98ca7f0
Merge branch 'mrschmidt/getbytes' of github.com:firebase/firebase-js-…
schmidt-sebastian Nov 10, 2021
15b1413
Lint
schmidt-sebastian Nov 10, 2021
d119bfb
Typo
schmidt-sebastian Nov 11, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/clever-eggs-relate.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@firebase/storage": minor
"firebase": minor
---

Adds `getBytes()`, `getStream()` and `getBlob()`, which allow direct file downloads from the SDK.
14 changes: 12 additions & 2 deletions common/api-review/storage.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -77,14 +77,15 @@ export class _FirebaseStorageImpl implements FirebaseStorage {
_getAuthToken(): Promise<string | null>;
get host(): string;
set host(host: string);
// Warning: (ae-forgotten-export) The symbol "ConnectionType" needs to be exported by the entry point index.d.ts
// Warning: (ae-forgotten-export) The symbol "RequestInfo" needs to be exported by the entry point index.d.ts
// Warning: (ae-forgotten-export) The symbol "Connection" needs to be exported by the entry point index.d.ts
// Warning: (ae-forgotten-export) The symbol "Request" needs to be exported by the entry point index.d.ts
//
// (undocumented)
_makeRequest<T>(requestInfo: RequestInfo_2<T>, requestFactory: () => Connection, authToken: string | null, appCheckToken: string | null): Request_2<T>;
_makeRequest<I extends ConnectionType, O>(requestInfo: RequestInfo_2<I, O>, requestFactory: () => Connection<I>, authToken: string | null, appCheckToken: string | null): Request_2<O>;
// (undocumented)
makeRequestWithTokens<T>(requestInfo: RequestInfo_2<T>, requestFactory: () => Connection): Promise<T>;
makeRequestWithTokens<I extends ConnectionType, O>(requestInfo: RequestInfo_2<I, O>, requestFactory: () => Connection<I>): Promise<O>;
_makeStorageReference(loc: _Location): _Reference;
get maxOperationRetryTime(): number;
set maxOperationRetryTime(time: number);
Expand Down Expand Up @@ -112,6 +113,12 @@ export interface FullMetadata extends UploadMetadata {
updated: string;
}

// @public
export function getBlob(ref: StorageReference, maxDownloadSizeBytes?: number): Promise<Blob>;

// @public
export function getBytes(ref: StorageReference, maxDownloadSizeBytes?: number): Promise<ArrayBuffer>;

// @internal (undocumented)
export function _getChild(ref: StorageReference, childPath: string): _Reference;

Expand All @@ -124,6 +131,9 @@ export function getMetadata(ref: StorageReference): Promise<FullMetadata>;
// @public
export function getStorage(app?: FirebaseApp, bucketUrl?: string): FirebaseStorage;

// @public
export function getStream(ref: StorageReference, maxDownloadSizeBytes?: number): NodeJS.ReadableStream;

// Warning: (ae-forgotten-export) The symbol "StorageError" needs to be exported by the entry point index.d.ts
//
// @internal (undocumented)
Expand Down
4 changes: 2 additions & 2 deletions packages/storage/.run/All Tests.run.xml
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@
<env name="TS_NODE_CACHE" value="NO" />
</envs>
<ui>bdd</ui>
<extra-mocha-options>--require ts-node/register/type-check --require index.node.ts</extra-mocha-options>
<extra-mocha-options>--require ts-node/register/type-check --require src/index.node.ts</extra-mocha-options>
<test-kind>PATTERN</test-kind>
<test-pattern>test/{,!(browser)/**/}*.test.ts</test-pattern>
<method v="2" />
</configuration>
</component>
</component>
15 changes: 6 additions & 9 deletions packages/storage/src/api.browser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
*/

import { StorageReference } from './public-types';
import { Reference, getBlobInternal } from './reference';
import { getModularInstance } from '@firebase/util';

/**
* Downloads the data at the object's location. Returns an error if the object
Expand All @@ -33,12 +35,12 @@ import { StorageReference } from './public-types';
* retrieve.
* @returns A Promise that resolves with a Blob containing the object's bytes
*/
// eslint-disable-next-line @typescript-eslint/no-unused-vars
function getBlob(
export function getBlob(
ref: StorageReference,
maxDownloadSizeBytes?: number
): Promise<Blob> {
throw new Error('Not implemented');
ref = getModularInstance(ref);
return getBlobInternal(ref as Reference, maxDownloadSizeBytes);
}

/**
Expand All @@ -53,14 +55,9 @@ function getBlob(
* retrieve.
* @returns A stream with the object's data as bytes
*/
// eslint-disable-next-line @typescript-eslint/no-unused-vars
function getStream(
export function getStream(
ref: StorageReference,
maxDownloadSizeBytes?: number
): NodeJS.ReadableStream {
throw new Error('getStream() is only supported by NodeJS builds');
}

// TODO(getbytes): Export getBlob/getStream

export {};
14 changes: 6 additions & 8 deletions packages/storage/src/api.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
*/

import { StorageReference } from './public-types';
import { Reference, getStreamInternal } from './reference';
import { getModularInstance } from '@firebase/util';

/**
* Downloads the data at the object's location. Returns an error if the object
Expand All @@ -34,7 +36,7 @@ import { StorageReference } from './public-types';
* @returns A Promise that resolves with a Blob containing the object's bytes
*/
// eslint-disable-next-line @typescript-eslint/no-unused-vars
function getBlob(
export function getBlob(
ref: StorageReference,
maxDownloadSizeBytes?: number
): Promise<Blob> {
Expand All @@ -53,14 +55,10 @@ function getBlob(
* retrieve.
* @returns A stream with the object's data as bytes
*/
// eslint-disable-next-line @typescript-eslint/no-unused-vars
function getStream(
export function getStream(
ref: StorageReference,
maxDownloadSizeBytes?: number
): NodeJS.ReadableStream {
throw new Error('Not implemented');
ref = getModularInstance(ref);
return getStreamInternal(ref as Reference, maxDownloadSizeBytes);
}

// TODO(getbytes): Export getBlob/getStream

export {};
25 changes: 24 additions & 1 deletion packages/storage/src/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ import {
getDownloadURL as getDownloadURLInternal,
deleteObject as deleteObjectInternal,
Reference,
_getChild as _getChildInternal
_getChild as _getChildInternal,
getBytesInternal
} from './reference';
import { STORAGE_TYPE } from './constants';
import { EmulatorMockTokenOptions, getModularInstance } from '@firebase/util';
Expand Down Expand Up @@ -76,6 +77,28 @@ export {
} from './implementation/taskenums';
export { StringFormat };

/**
* Downloads the data at the object's location. Returns an error if the object
* is not found.
*
* To use this functionality, you have to whitelist your app's origin in your
* Cloud Storage bucket. See also
* https://cloud.google.com/storage/docs/configuring-cors
*
* @public
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@egilmorez can you please scan the PR and review the comments with the @public tag? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping @markarndt could give that a go . . . but if he's too busy and there's urgency, I can take a look this afternoon for sure.

* @param ref - StorageReference where data should be download.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"..should be downloaded."

* @param maxDownloadSizeBytes - If set, the maximum allowed size in bytes to
* retrieve.
* @returns A Promise containing the object's bytes
*/
export function getBytes(
ref: StorageReference,
maxDownloadSizeBytes?: number
): Promise<ArrayBuffer> {
ref = getModularInstance(ref);
return getBytesInternal(ref as Reference, maxDownloadSizeBytes);
}

/**
* Uploads data to this object's location.
* The upload is not resumable.
Expand Down
23 changes: 19 additions & 4 deletions packages/storage/src/implementation/connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,27 @@
/** Network headers */
export type Headers = Record<string, string>;

/** Response type exposed by the networking APIs. */
export type ConnectionType =
| string
| ArrayBuffer
| Blob
| NodeJS.ReadableStream;

/**
* A lightweight wrapper around XMLHttpRequest with a
* goog.net.XhrIo-like interface.
*
* You can create a new connection by invoking `newTextConnection()`,
* `newBytesConnection()` or `newStreamConnection()`.
*/
export interface Connection {
export interface Connection<T extends ConnectionType> {
/**
* This method should never reject. In case of encountering an error, set an error code internally which can be accessed
* by calling getErrorCode() by callers.
* Sends a request to the provided URL.
*
* This method never rejects its promise. In case of encountering an error,
* it sets an error code internally which can be accessed by calling
* getErrorCode() by callers.
*/
send(
url: string,
Expand All @@ -38,7 +51,9 @@ export interface Connection {

getStatus(): number;

getResponseText(): string;
getResponse(): T;

getErrorText(): string;

/**
* Abort the request.
Expand Down
65 changes: 32 additions & 33 deletions packages/storage/src/implementation/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,12 @@
* abstract representations.
*/

import { start, stop, id as backoffId } from './backoff';
import {
StorageError,
unknown,
appDeleted,
canceled,
retryLimitExceeded
} from './error';
import { RequestHandler, RequestInfo } from './requestinfo';
import { id as backoffId, start, stop } from './backoff';
import { appDeleted, canceled, retryLimitExceeded, unknown } from './error';
import { ErrorHandler, RequestHandler, RequestInfo } from './requestinfo';
import { isJustDef } from './type';
import { makeQueryString } from './url';
import { Headers, Connection, ErrorCode } from './connection';
import { Connection, ErrorCode, Headers, ConnectionType } from './connection';

export interface Request<T> {
getPromise(): Promise<T>;
Expand All @@ -46,15 +40,23 @@ export interface Request<T> {
cancel(appDelete?: boolean): void;
}

class NetworkRequest<T> implements Request<T> {
private pendingConnection_: Connection | null = null;
/**
* Handles network logic for all Storage Requests, including error reporting and
* retries with backoff.
*
* @param I - the type of the backend's network response.
* @param - O the output type used by the rest of the SDK. The conversion
* happens in the specified `callback_`.
*/
class NetworkRequest<I extends ConnectionType, O> implements Request<O> {
private pendingConnection_: Connection<I> | null = null;
private backoffId_: backoffId | null = null;
private resolve_!: (value?: T | PromiseLike<T>) => void;
private resolve_!: (value?: O | PromiseLike<O>) => void;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
private reject_!: (reason?: any) => void;
private canceled_: boolean = false;
private appDelete_: boolean = false;
promise_: Promise<T>;
private promise_: Promise<O>;

constructor(
private url_: string,
Expand All @@ -63,14 +65,14 @@ class NetworkRequest<T> implements Request<T> {
private body_: string | Blob | Uint8Array | null,
private successCodes_: number[],
private additionalRetryCodes_: number[],
private callback_: RequestHandler<string, T>,
private errorCallback_: RequestHandler<StorageError, StorageError> | null,
private callback_: RequestHandler<I, O>,
private errorCallback_: ErrorHandler | null,
private timeout_: number,
private progressCallback_: ((p1: number, p2: number) => void) | null,
private connectionFactory_: () => Connection
private connectionFactory_: () => Connection<I>
) {
this.promise_ = new Promise((resolve, reject) => {
this.resolve_ = resolve as (value?: T | PromiseLike<T>) => void;
this.resolve_ = resolve as (value?: O | PromiseLike<O>) => void;
this.reject_ = reject;
this.start_();
});
Expand Down Expand Up @@ -135,17 +137,14 @@ class NetworkRequest<T> implements Request<T> {
*/
const backoffDone: (
requestWentThrough: boolean,
status: RequestEndStatus
status: RequestEndStatus<I>
) => void = (requestWentThrough, status) => {
const resolve = this.resolve_;
const reject = this.reject_;
const connection = status.connection as Connection;
const connection = status.connection as Connection<I>;
if (status.wasSuccessCode) {
try {
const result = this.callback_(
connection,
connection.getResponseText()
);
const result = this.callback_(connection, connection.getResponse());
if (isJustDef(result)) {
resolve(result);
} else {
Expand All @@ -157,7 +156,7 @@ class NetworkRequest<T> implements Request<T> {
} else {
if (connection !== null) {
const err = unknown();
err.serverResponse = connection.getResponseText();
err.serverResponse = connection.getErrorText();
if (this.errorCallback_) {
reject(this.errorCallback_(connection, err));
} else {
Expand All @@ -182,7 +181,7 @@ class NetworkRequest<T> implements Request<T> {
}

/** @inheritDoc */
getPromise(): Promise<T> {
getPromise(): Promise<O> {
return this.promise_;
}

Expand Down Expand Up @@ -219,15 +218,15 @@ class NetworkRequest<T> implements Request<T> {
* A collection of information about the result of a network request.
* @param opt_canceled - Defaults to false.
*/
export class RequestEndStatus {
export class RequestEndStatus<I extends ConnectionType> {
/**
* True if the request was canceled.
*/
canceled: boolean;

constructor(
public wasSuccessCode: boolean,
public connection: Connection | null,
public connection: Connection<I> | null,
canceled?: boolean
) {
this.canceled = !!canceled;
Expand Down Expand Up @@ -266,22 +265,22 @@ export function addAppCheckHeader_(
}
}

export function makeRequest<T>(
requestInfo: RequestInfo<T>,
export function makeRequest<I extends ConnectionType, O>(
requestInfo: RequestInfo<I, O>,
appId: string | null,
authToken: string | null,
appCheckToken: string | null,
requestFactory: () => Connection,
requestFactory: () => Connection<I>,
firebaseVersion?: string
): Request<T> {
): Request<O> {
const queryPart = makeQueryString(requestInfo.urlParams);
const url = requestInfo.url + queryPart;
const headers = Object.assign({}, requestInfo.headers);
addGmpidHeader_(headers, appId);
addAuthHeader_(headers, authToken);
addVersionHeader_(headers, firebaseVersion);
addAppCheckHeader_(headers, appCheckToken);
return new NetworkRequest<T>(
return new NetworkRequest<I, O>(
url,
requestInfo.method,
headers,
Expand Down
Loading