fix: improve the types on acl.ts (#459)

Author: JustinBeckwith

handwritten/storage/src/acl.ts

@@ -23,6 +23,35 @@ import * as is from 'is';
 import {DecorateRequestOptions, BodyResponseCallback} from '@google-cloud/common';
 import * as r from 'request';
 
+export interface AclOptions {
+  pathPrefix: string;
+  request: typeof r;
+}
+
+export type GetAclResponse =
+    [AccessControlObject | AccessControlObject[], r.Response];
+export interface GetAclCallback {
+  (err: Error|null, acl?: AccessControlObject|AccessControlObject[]|null,
+   apiResponse?: r.Response): void;
+}
+export interface GetAclOptions {
+  entity: string;
+  generation?: number;
+  userProject?: string;
+}
+
+export interface UpdateAclOptions {
+  entity: string;
+  role: string;
+  generation?: number;
+  userProject?: string;
+}
+export type UpdateAclResponse = [AccessControlObject, r.Response];
+export interface UpdateAclCallback {
+  (err: Error|null, acl?: AccessControlObject|null,
+   apiResponse?: r.Response): void;
+}
+
 export interface AddAclOptions {
   entity: string;
   role: string;
@@ -38,13 +67,18 @@ export type RemoveAclResponse = [r.Response];
 export interface RemoveAclCallback {
   (err: Error|null, apiResponse?: r.Response): void;
 }
+export interface RemoveAclOptions {
+  entity: string;
+  generation?: number;
+  userProject?: string;
+}
 
 interface AclQuery {
   generation: number;
   userProject: string;
 }
 
-interface AccessControlObject {
+export interface AccessControlObject {
   entity: string;
   role: string;
   projectTeam: string;
@@ -243,7 +277,8 @@ class AclRoleAccessorMethods {
     const entities = AclRoleAccessorMethods.entities;
     const roleGroup = role.toLowerCase() + 's';
 
-    this[roleGroup] = entities.reduce((acc, entity) => {
+    // tslint:disable-next-line:no-any
+    (this as any)[roleGroup] = entities.reduce((acc, entity) => {
       const isPrefix = entity.charAt(entity.length - 1) === '-';
 
       accessMethods.forEach(accessMethod => {
@@ -255,39 +290,42 @@ class AclRoleAccessorMethods {
 
         // Wrap the parent accessor method (e.g. `add` or `delete`) to avoid the
         // more complex API of specifying an `entity` and `role`.
-        acc[method] = (entityId, options, callback) => {
-          let apiEntity;
-
-          if (is.fn(options)) {
-            callback = options;
-            options = {};
-          }
-
-          if (isPrefix) {
-            apiEntity = entity + entityId;
-          } else {
-            // If the entity is not a prefix, it is a special entity group that
-            // does not require further details. The accessor methods only
-            // accept a callback.
-            apiEntity = entity;
-            callback = entityId;
-          }
-
-          options = extend(
-              {
-                entity: apiEntity,
-                role,
-              },
-              options);
-
-          const args = [options];
-
-          if (is.fn(callback)) {
-            args.push(callback);
-          }
-
-          return this[accessMethod].apply(this, args);
-        };
+        // tslint:disable-next-line:no-any
+        (acc as any)[method] =
+            (entityId: string, options: {}, callback: Function|{}) => {
+              let apiEntity;
+
+              if (is.fn(options)) {
+                callback = options;
+                options = {};
+              }
+
+              if (isPrefix) {
+                apiEntity = entity + entityId;
+              } else {
+                // If the entity is not a prefix, it is a special entity group
+                // that does not require further details. The accessor methods
+                // only accept a callback.
+                apiEntity = entity;
+                callback = entityId;
+              }
+
+              options = extend(
+                  {
+                    entity: apiEntity,
+                    role,
+                  },
+                  options);
+
+              const args = [options];
+
+              if (is.fn(callback)) {
+                args.push(callback);
+              }
+
+              // tslint:disable-next-line:no-any
+              return (this as any)[accessMethod].apply(this, args);
+            };
       });
 
       return acc;
@@ -339,9 +377,9 @@ class AclRoleAccessorMethods {
 class Acl extends AclRoleAccessorMethods {
   default !: Acl;
   pathPrefix: string;
-  request_;
+  request_: typeof r;
 
-  constructor(options) {
+  constructor(options: AclOptions) {
     super();
     this.pathPrefix = options.pathPrefix;
     this.request_ = options.request;
@@ -514,7 +552,10 @@ class Acl extends AclRoleAccessorMethods {
    * region_tag:storage_remove_file_owner
    * Example of removing an owner from a bucket:
    */
-  delete(options, callback?) {
+  delete(options: RemoveAclOptions): Promise<RemoveAclResponse>;
+  delete(options: RemoveAclOptions, callback: RemoveAclCallback): void;
+  delete(options: RemoveAclOptions, callback?: RemoveAclCallback):
+      void|Promise<RemoveAclResponse> {
     const query = {} as AclQuery;
 
     if (options.generation) {
@@ -619,14 +660,19 @@ class Acl extends AclRoleAccessorMethods {
    * region_tag:storage_print_bucket_acl_for_user
    * Example of printing a bucket's ACL for a specific user:
    */
-  get(options, callback?) {
+  get(options?: GetAclOptions): Promise<GetAclResponse>;
+  get(options: GetAclOptions, callback: GetAclCallback): void;
+  get(callback: GetAclCallback): void;
+  get(optionsOrCallback?: GetAclOptions|GetAclCallback,
+      cb?: GetAclCallback): void|Promise<GetAclResponse> {
+    const options =
+        typeof optionsOrCallback === 'object' ? optionsOrCallback : null;
+    const callback =
+        typeof optionsOrCallback === 'function' ? optionsOrCallback : cb;
     let path = '';
     const query = {} as AclQuery;
 
-    if (is.fn(options)) {
-      callback = options;
-      options = null;
-    } else {
+    if (options) {
       path = '/' + encodeURIComponent(options.entity);
 
       if (options.generation) {
@@ -718,7 +764,10 @@ class Acl extends AclRoleAccessorMethods {
    *   const apiResponse = data[1];
    * });
    */
-  update(options, callback?) {
+  update(options: UpdateAclOptions): Promise<UpdateAclResponse>;
+  update(options: UpdateAclOptions, callback: UpdateAclCallback): void;
+  update(options: UpdateAclOptions, callback?: UpdateAclCallback):
+      void|Promise<UpdateAclResponse> {
     const query = {} as AclQuery;
 
     if (options.generation) {
@@ -753,7 +802,8 @@ class Acl extends AclRoleAccessorMethods {
    *
    * @private
    */
-  makeAclObject_(accessControlObject): AccessControlObject {
+  makeAclObject_(accessControlObject: AccessControlObject):
+      AccessControlObject {
     const obj = {
       entity: accessControlObject.entity,
       role: accessControlObject.role,

handwritten/storage/src/index.ts

@@ -29,6 +29,7 @@ import {Bucket} from './bucket';
 import {Channel} from './channel';
 import {File} from './file';
 import {normalize} from './util';
+import {AccessControlObject} from './acl';
 
 export interface GetServiceAccountOptions {
   userProject?: string;
@@ -764,4 +765,4 @@ promisifyAll(Storage, {
  */
 export {Storage};
 
-export {Bucket, File, Channel};
+export {Bucket, File, Channel, AccessControlObject};

handwritten/storage/system-test/storage.ts

@@ -28,10 +28,9 @@ import * as path from 'path';
 import * as through from 'through2';
 import * as tmp from 'tmp';
 import * as uuid from 'uuid';
-import * as r from 'request';
 import {util, ApiError, InstanceResponseCallback, BodyResponseCallback} from '@google-cloud/common';
-import {Storage, Bucket, File} from '../src';
-import {DeleteBucketCallback, UploadCallback} from '../src/bucket';
+import {Storage, Bucket, File, AccessControlObject} from '../src';
+import {DeleteBucketCallback} from '../src/bucket';
 import * as nock from 'nock';
 import {DeleteFileCallback} from '../src/file';
 
@@ -208,7 +207,8 @@ describe('storage', () => {
                   (err, accessControl) => {
                     assert.ifError(err);
                     assert.strictEqual(
-                        accessControl.role, storage.acl.OWNER_ROLE);
+                        (accessControl as AccessControlObject)!.role,
+                        storage.acl.OWNER_ROLE);
 
                     bucket.acl.default.update(
                         {
@@ -218,7 +218,8 @@ describe('storage', () => {
                         (err, accessControl) => {
                           assert.ifError(err);
                           assert.strictEqual(
-                              accessControl.role, storage.acl.READER_ROLE);
+                              (accessControl as AccessControlObject).role,
+                              storage.acl.READER_ROLE);
 
                           bucket.acl.default.delete(
                               {entity: USER_ACCOUNT}, done);
@@ -249,7 +250,9 @@ describe('storage', () => {
 
               bucket.acl.get(opts, (err, accessControl) => {
                 assert.ifError(err);
-                assert.strictEqual(accessControl.role, storage.acl.OWNER_ROLE);
+                assert.strictEqual(
+                    (accessControl as AccessControlObject).role,
+                    storage.acl.OWNER_ROLE);
 
                 bucket.acl.delete(opts, done);
               });
@@ -274,7 +277,7 @@ describe('storage', () => {
                   (err, accessControl) => {
                     assert.ifError(err);
                     assert.strictEqual(
-                        accessControl.role, storage.acl.WRITER_ROLE);
+                        accessControl!.role, storage.acl.WRITER_ROLE);
 
                     bucket.acl.delete({entity: USER_ACCOUNT}, done);
                   });
@@ -343,8 +346,8 @@ describe('storage', () => {
           bucket.makePrivate(err => {
             assert.ifError(err);
             bucket.acl.get({entity: 'allUsers'}, (err, aclObject) => {
-              assert.strictEqual(err.code, 404);
-              assert.strictEqual(err.message, 'Not Found');
+              assert.strictEqual((err as ApiError).code, 404);
+              assert.strictEqual(err!.message, 'Not Found');
               assert.strictEqual(aclObject, null);
               done();
             });
@@ -577,8 +580,8 @@ describe('storage', () => {
               assert.ifError(err);
 
               file!.acl.get({entity: 'allUsers'}, (err, aclObject) => {
-                assert.strictEqual(err.code, 404);
-                assert.strictEqual(err.message, 'Not Found');
+                assert.strictEqual((err as ApiError)!.code, 404);
+                assert.strictEqual(err!.message, 'Not Found');
                 assert.strictEqual(aclObject, null);
                 done();
               });