Storage List API #1610

fredzqm · 2019-03-17T08:37:32Z

API proposal: internal golink firebase-storage-list-api

… into fz/storage-list

schmidt-sebastian

Thanks! This looks pretty good. Most of my comments are style nits and suggestions for improved typings.

packages/storage-types/index.d.ts

schmidt-sebastian · 2019-03-19T14:38:57Z

packages/storage-types/index.d.ts

+}
+
+export interface ListOptions {
+  maxResults?: number | null | undefined;


The question mark already implies undefined. You should get rid of either the question mark or the undefined. I lean towards keeping the question mark.

Good point. Fixed

schmidt-sebastian · 2019-03-19T14:42:05Z

packages/storage/src/implementation/list.ts

@@ -0,0 +1,90 @@
+/**
+ * @license
+ * Copyright 2017 Google Inc.


Looks like someone is really dwelling in the past here :)

schmidt-sebastian · 2019-03-19T14:47:29Z

packages/storage/src/implementation/list.ts

+import * as type from './type';
+import { ListResult } from '../list';
+
+export function fromResource(


Do you mind adding a comment to explain what a resource is in this context? Is it always of type ListResponse?

Alternatively, it might make sense to use more specific names. You should then add a type definition that describes the expected resource type:

interface Resource { nextPageToken?: string, ... }

It also looks like this function is only used in this file and does not need to be exported.

schmidt-sebastian · 2019-03-19T14:49:45Z

packages/storage/src/implementation/list.ts

+
+export function fromResource(
+  authWrapper: AuthWrapper,
+  resource: { [name: string]: any }


It's best to avoid any additional usages of any. Can you change this any to unknown? unknown still keeps type safety, which will also make the code easier to follow.

Done. I was borrowing the examples in metadata.ts. It is much more readable with types.

schmidt-sebastian · 2019-03-19T15:12:20Z

packages/storage/src/reference.ts

@@ -201,6 +203,28 @@ export class Reference {
    });
  }

+  /**
+   * @param options.maxResults limits the total number of prefixes and items to return..
+   * @param options.pageToken nextPageToken from a previous list(). Resume from that position.


Maybe change this to:

'The nextPageToken from a previous list() response. If provided, listing is resumed from that position.

schmidt-sebastian · 2019-03-19T15:12:42Z

packages/storage/src/reference.ts

@@ -201,6 +203,28 @@ export class Reference {
    });
  }

+  /**
+   * @param options.maxResults limits the total number of prefixes and items to return..


Maybe prefix with If set, limits...

schmidt-sebastian · 2019-03-19T15:15:21Z

packages/storage/test/requests.test.ts

-    for (let i = 0; i < maps.length; i++) {
-      const location = maps[i][0] as Location;
-      const url = maps[i][1] as string;
+    for (const d of maps) {


While you are here, you could make this:

for (const [location, url ] of maps) {

packages/storage/test/requests.test.ts

fredzqm · 2019-03-19T18:42:34Z

packages/storage/src/implementation/list.ts

+import * as type from './type';
+import { ListResult } from '../list';
+
+export function fromResource(


fredzqm · 2019-03-19T18:42:48Z

packages/storage/src/implementation/list.ts

@@ -0,0 +1,90 @@
+/**
+ * @license
+ * Copyright 2017 Google Inc.


fredzqm · 2019-03-19T18:43:46Z

packages/storage/src/implementation/list.ts

+
+export function fromResource(
+  authWrapper: AuthWrapper,
+  resource: { [name: string]: any }


Done. I was borrowing the examples in metadata.ts. It is much more readable with types.

fredzqm · 2019-03-19T18:45:10Z

packages/storage/src/implementation/list.ts

+  if (resource[prefixesKey]) {
+    for (const path of resource[prefixesKey]) {
+      const reference = authWrapper.makeStorageReference(
+        new Location(authWrapper.bucket(), path.replace(/\/$/, ''))


fredzqm · 2019-03-19T18:46:20Z

packages/storage/src/implementation/list.ts

+  if (!validType) {
+    throw 'Expected ListOptions object.';
+  }
+  const maxResultsKey = 'maxResults';


packages/storage/test/requests.test.ts

packages/storage-types/index.d.ts

fredzqm · 2019-03-19T19:03:17Z

packages/storage-types/index.d.ts

+}
+
+export interface ListOptions {
+  maxResults?: number | null | undefined;


Good point. Fixed

packages/firebase/index.d.ts

googlebot · 2019-05-24T22:35:39Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that 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 here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-05-25T01:15:16Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Feiyang1 · 2019-05-28T18:45:31Z

packages/firebase/index.d.ts

+     * @param options.maxResults If set, limits the total number of `prefixes`
+     *      and `items` to return. The default and maximum maxResults is 1000.
+     *      Use the nextPageToken to retrieve more objects.
+     * @param options.pageToken The `nextPageToken` from a previous call to


This style of describing the parameter only works if you define the type directly in this method, like the following:

list(options?: {maxResults: number | null, pageToken: string | null}

If an interface is used, the comments should coexist with the interface definition.

Since you have defined an interface, can you move the comments to the interface?

Done. Updated the doc.

Feiyang1 · 2019-05-28T18:51:34Z

scripts/docgen/content-sources/node/toc.yaml

@@ -124,3 +124,25 @@ toc:
    path: /docs/reference/node/firebase.firestore.Transaction
  - title: "WriteBatch"
    path: /docs/reference/node/firebase.firestore.WriteBatch
+
+- title: "firebase.storage"


I don't think storage is supported in Nodejs.

I see. I will revert it.
OOC, any reason we did not have it in the first place?

List API

e52cc85

fredzqm requested a review from schmidt-sebastian as a code owner March 17, 2019 08:37

fredzqm changed the title ~~List API~~ Storage List API Mar 17, 2019

fredzqm added 7 commits March 17, 2019 01:38

Update constants.ts

974b248

always pass in prefix

0984bc3

Merge branch 'fz/storage-list' of github.com:firebase/firebase-js-sdk…

324a7ba

… into fz/storage-list

clean up

1f52804

more tests and clean up

29ebc7c

more tests

3071b84

[AUTOMATED]: Prettier Code Styling

d1dad43

fredzqm requested review from Feiyang1, hiranya911 and hsubox76 as code owners March 17, 2019 10:06

fredzqm added 11 commits March 17, 2019 04:18

fix test

b3585cb

[AUTOMATED]: Prettier Code Styling

25b6169

Update .gitignore

bedf0f8

Update .gitignore

189184b

Update constants.ts

dd8a2a7

Update list.ts

99954da

Update reference.ts

931eefd

Merge branch 'fz/storage-list' of github.com:firebase/firebase-js-sdk…

1c3ab63

… into fz/storage-list

clean up

a11ab95

[AUTOMATED]: Prettier Code Styling

7e004b1

m

1bc3aee

schmidt-sebastian reviewed Mar 19, 2019

View reviewed changes

fredzqm added 3 commits March 19, 2019 12:05

address comments

49ec291

[AUTOMATED]: Prettier Code Styling

2cd9edd

docs

0a86471

fredzqm commented Mar 19, 2019

View reviewed changes

fredzqm assigned schmidt-sebastian Mar 19, 2019

fredzqm requested review from yuchenshi and zijianjoy as code owners May 24, 2019 22:30

wilhuff force-pushed the fz/storage-list branch from b2d4c76 to dff0f21 Compare May 25, 2019 01:15

zijianjoy removed request for jamesdaniels, mikelehen, dwoffinden, mmermerkaya, ryanpbrewster, davideast, jsdt, andirayo, alikn, bojeil-google, scottcrossen, pinarx, yuchenshi, wti806 and zijianjoy May 25, 2019 01:47

fredzqm added 2 commits May 28, 2019 07:46

mention rules version 2

0fee0bb

Merge remote-tracking branch 'origin/master' into fz/storage-list

21d5f83

Feiyang1 suggested changes May 28, 2019

View reviewed changes

fredzqm added 2 commits May 28, 2019 12:36

address comments

3f50d88

Update toc.yaml

4470649

Feiyang1 approved these changes May 28, 2019

View reviewed changes

fredzqm merged commit bc4a844 into master May 28, 2019

fredzqm mentioned this pull request May 28, 2019

Add listAll() to storage-types #1829

Merged

firebase locked and limited conversation to collaborators Oct 12, 2019

fredzqm deleted the fz/storage-list branch August 25, 2020 19:38

Storage List API #1610

Storage List API #1610

Uh oh!

Conversation

fredzqm commented Mar 17, 2019

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

googlebot commented May 24, 2019

Uh oh!

googlebot commented May 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredzqm May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fredzqm May 28, 2019 •

edited

Loading