feat(storage): Add getDownloadUrl method to the Storage API#2036
Conversation
| if (!metadata?.metadata?.firebaseStorageDownloadTokens) { | ||
| throw new FirebaseError({ | ||
| code: 'storage/no-download-token', | ||
| message: 'No download token available. Please create one in the Firebase Console.', |
There was a problem hiding this comment.
we actually do have a token API that the console uses. We would have to implement it here as well but perhaps we could point users to that method instead of the console
lepatryk
left a comment
There was a problem hiding this comment.
Thanks Maneesh for putting it together. I really think our customers would appreciate it.
However our download url design is a hot mess right now and I'm hoping we can clean it up in 2023. This means getDownloadUrl() in the current form will either be deprecated or at least discouraged.
Does it make sense to add it here if we are planning to deprecate it a few months later?
| // Gets metadata from firebase backend instead of GCS | ||
| getFirebaseMetadata(): Promise<FirebaseMetadata> { | ||
| // We need to talk to the firebase storage endpoints instead of the google cloud bucket endpoint | ||
| const endpoint = "https://firebasestorage.googleapis.com/v0"; |
There was a problem hiding this comment.
How can configure this for testing against our internal testing endpoints or emulator?
There was a problem hiding this comment.
The expectation is that you call getStorage(app, 'myendpoint') or getStorage(undefined, 'myEndpoint')
| constructor( | ||
| bucket: FirebaseStorageBucket, | ||
| name: string, | ||
| private endpoint: string, |
There was a problem hiding this comment.
I'm curious how other SDKs handle env overrides. Could you confirm this is commonly how this is implemented?
There was a problem hiding this comment.
Firestore just uses the google cloud package and does something like this:
const settings: Settings = {
host: "localhost:8080",
ssl: false
};
// Initialize Firestore
const firestore = new Firestore(settings);
For RTDB, we ask them to override the databaseUrl:
admin.initializeApp({
credential: admin.credential.cert(serviceAccount),
databaseURL: "https://movie-picker-729bb-default-rtdb.firebaseio.com",
});
So I don't think there's a best practices way to get this done. My recommendation is to do something like:
getStorage({ host: 'myhosturl' });
So that we can allow for more options to be customized down the road. WDYT?
EDIT: Introducing a new ENV var seems cleaner than piping through a bunch of classes. WDYT?
There was a problem hiding this comment.
Ack, I don't have a strong sense of what design is right here, env var sounds fine.
Eventually these patterns should be consolidated imo, but that can happen async.
| * Gets metadata from firebase backend instead of GCS | ||
| * @returns {FirebaseMetadata} | ||
| */ | ||
| getFirebaseMetadata(): Promise<FirebaseMetadata> { |
| }); | ||
| } | ||
| const [token] = downloadTokens.split(","); | ||
| return `${this.endpoint}/v0/b/${this.bucket.name}/o/${encodeURIComponent( |
There was a problem hiding this comment.
seems like there's a bug here, getFirebaseMetadata above assumes this.endpoint has the /v0 url part.
| this.endpoint = endpoint; | ||
| } | ||
| /** | ||
| * |
There was a problem hiding this comment.
Did you want me to add more information about this?
| super(bucket, name, options); | ||
| } | ||
| /** | ||
| * Gets metadata from firebase backend instead of GCS |
|
|
||
| process.env.STORAGE_EMULATOR_HOST = `http://${process.env.FIREBASE_STORAGE_EMULATOR_HOST}`; | ||
| } | ||
| this.endpoint = (userEndpoint || process.env.STORAGE_EMULATOR_HOST || 'https://firebasestorage.googleapis.com') + '/v0'; |
There was a problem hiding this comment.
Since we already have one for the emulator host, does it make sense to use another env var for the endpoint override?
| uri, | ||
| }, | ||
| (err, body) => { | ||
| console.log(body); |
| * @returns {FirebaseMetadata} | ||
| */ | ||
| private getFirebaseMetadata(): Promise<FirebaseMetadata> { | ||
| // Build any custom headers based on the defined interceptors on the parent |
| } | ||
|
|
||
| /** | ||
| * Gets the download URL for a given file. Will throw a `FirebaseError` if there are no download tokens available. |
There was a problem hiding this comment.
|
|
||
| /** | ||
| * Gets the download URL for a given file. Will throw a `FirebaseError` if there are no download tokens available. | ||
| * @returns {Promise<string>} |
There was a problem hiding this comment.
Please update with a description of the result or remove as this is redundant with the method signature
| export class FirebaseStorageClient extends StorageClient { | ||
| /** | ||
| * | ||
| * @param bucketName |
| */ | ||
| export class FirebaseStorageBucket extends Bucket { | ||
| /** | ||
| * @param name |
tonyjhuang
left a comment
There was a problem hiding this comment.
overall direction lgtm, small nits, can you go through the whole PR to make sure its up to date?
| } | ||
| /** | ||
| * Gets metadata from Firebase backend instead of GCS | ||
| * @returns {FirebaseMetadata} |
There was a problem hiding this comment.
small nit: jsdoc clauses unnecessary unless they add context that cant be inferred from the method signature
go/java-practices/javadoc#param
|
|
||
| /** | ||
| * Gets the download URL for a given file. Will throw a `FirebaseError` if there are no download tokens available. | ||
| * @returns {Promise<string>} |
| endpoint: string, | ||
| file: File | ||
| ): Promise<FirebaseMetadata> { | ||
| // Build any custom headers based on the defined interceptors on the parent |
| export class Storage { | ||
| get app(): App; | ||
| bucket(name?: string): Bucket; | ||
| // Warning: (ae-forgotten-export) The symbol "FirebaseStorageBucket" needs to be exported by the entry point index.d.ts |
| return verifyBucket(bucket, 'storage().bucket(string)') | ||
| .should.eventually.be.fulfilled; | ||
| }); | ||
| it('bucket(string) creates a download token', async () => { |
Fixes #1352