Skip to content
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

Incorrect type for drive.files.export #2406

Closed
aryzing opened this issue Oct 18, 2020 · 2 comments · Fixed by #2412
Closed

Incorrect type for drive.files.export #2406

aryzing opened this issue Oct 18, 2020 · 2 comments · Fixed by #2412
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@aryzing
Copy link

aryzing commented Oct 18, 2020

  1. Is this a client library issue or a product issue?
    client library issue

  2. Did someone already solve this?
    no

  3. Do you have a support contract?
    no

Environment details

  • OS: Linux 5.4 Ubuntu 18.04.5 LTS (Bionic Beaver)
  • Node.js version: 14.4.0
  • npm version: 6.14.5
  • googleapis version: 61.0.0

Steps to reproduce

The drive.files.export function is currently typed as

        export(params: Params$Resource$Files$Export, options: StreamMethodOptions): GaxiosPromise<Readable>;
        export(params?: Params$Resource$Files$Export, options?: MethodOptions): GaxiosPromise<void>;
        export(params: Params$Resource$Files$Export, options: StreamMethodOptions | BodyResponseCallback<Readable>, callback: BodyResponseCallback<Readable>): void;
        export(params: Params$Resource$Files$Export, options: MethodOptions | BodyResponseCallback<void>, callback: BodyResponseCallback<void>): void;
        export(params: Params$Resource$Files$Export, callback: BodyResponseCallback<void>): void;
        export(callback: BodyResponseCallback<void>): void;

From the types above, at least the second one is incorrectly typed. It's typed as returning GaxiosPromise<void>, yet data is clearly available in the return value. I'm having to cast data to the correct type to get around the types defined above.

const { data } = await drive.files.export({
  fileId: 'foo',
  mimeType: 'text/plain',
});

/**
 * Casting `data` to unknown b/c it's typed as `void` despite having found
 * it to contain a string at runtime.
 *
 * E.g., `console.log(typeof data) // string`.
 */
const unknownData: unknown = data;

if (typeof unknownData !== 'string') {
  if (unknownData === undefined || unknownData === null) {
    return { content: '' };
  } else {
    throw new Error('Unexpected data type');
  }
}

return { content: unknownData };
@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Oct 19, 2020
@sofisl sofisl added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Oct 19, 2020
@yoshi-automation yoshi-automation removed the triage me I really want to be triaged. label Oct 19, 2020
@JustinBeckwith
Copy link
Contributor

JustinBeckwith commented Oct 20, 2020

This is an actual bug with the TypeScript types in the generator! It looks like the discovery doc for drive.files.export has this field:

"supportsMediaDownload": true,

Right now the generator only looks for the response field when figuring out the return type for the API call. If the supportsMediaDownload type is set to true, then the API probably needs to return GaxiosPromise<T = any>. The reason we can't figure out T automatically is that the user is responsible for setting the mime type, which deeply impacts the shape of the data returned. So the usage likely should be something like:

const res = await drive.files.export<string>({
  fileId: '12345',
  mimeType: 'text/plain'
});

The <string> here lets the compiler know we want to return a string instead of void. Alternatively, you could just grab the stream.

after discussion

We decided that we have no idea what this type will be in many cases, and different APIs expose different ways of setting the mimeType, if they expose it at all. Decision: in cases where supportsMediaDownload is set to true, we will type the GaxiosPromise as unknown or any (need to research a bit on the best one to use). At least then, users only need to do a single cast to their preferred type, instead of casting to unknown and then finally casting to the real type.

@aryzing
Copy link
Author

aryzing commented Oct 21, 2020

Thanks for taking a look at this so quickly. As far as unknown vs any, the clue is in

we have no idea what this type will be

In other words, the type is unknown ;)

any just serves as a compiler opt-out.

Looking forward to changes, and thanks again for looking into this so quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants