Google-api-nodejs-client: `gaxios` treats `arraybuffer` differently from `axios`

[rdmurphy]

Thanks for stopping by to let us know something could be better!

PLEASE READ: If you have a support contract with Google, please create an issue in the support console instead of filing on GitHub. This will ensure a timely response.

Please run down the following list and make sure you've tried the usual "quick fixes":

• Search the issues already opened: https://github.com/googleapis/google-api-nodejs-client/issues
• Search the issues on our "catch-all" repository: https://github.com/googleapis/google-cloud-node
• Search StackOverflow: http://stackoverflow.com/questions/tagged/google-cloud-platform+node.js

If you are still having issues, please be sure to include as much information as possible:

Environment details

• OS: MacOS
• Node.js version: 10.5.0
• npm version: 6.7.0
• googleapis version: 37.0.0

Steps to reproduce

With the move to gaxios from axios, there's a few things that have changed with regard to how node-fetch handles arraybuffer differently from axios. (Mainly, it does nothing to it at all 🙃 .) I will say that this is not necessarily a bug, but for anyone else who depends on this to run drive.files.export calls may be surprised the exported files stop working.

In axios, it will convert an ArrayBuffer to a Buffer as part of the request, but node-fetch leaves that up to the user.

So in pre-37.0.0 land, this would work fine (and return a Buffer at res.data):

const res = await drive.files.export(
  {
    fileId: spreadsheetId,
    mimeType:
      'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet',
  },
  { responseType: 'arraybuffer' }
);

But now in 37.0.0, you may have to pass that to Buffer.from before you can potentially use it.

Like I said above - this may not actually be a "bug" in the traditional sense, but seeing as I learned the tip of how to use arraybuffer with drive.files.export to pull down Excel files from the issues of this library, I figured I'd flag it just in case it's worth noting somewhere? (Just not sure... where?)

Thanks for the library!

[JustinBeckwith]

Thank you so much for digging into this! This... is a weird one. Looking at the source in axios, it looks like they treat responseType of blob and arrayBuffer exactly the same 😨 gaxios will return an ArrayBuffer (which you can Buffer.from) if you ask for an ArrayBuffer, and it will return a regular ol' Buffer if you ask for the blob.

As you're calling out - this is technically a breaking change, but I also think that what we have now is the correct behavior.

If you ask for a blob, does this give you a buffer that works how you'd expect?

[rdmurphy]

Of course!

I did experiment with passing in blob instead of arraybuffer when I was debugging but couldn't get it to work for my purposes, but that may be due to my fuzzy understanding of node-fetch's treatment of blob.

In my use case I'm taking the Excel file that drive.files.export returns and passing it to another function that understands how to interface with it (eventually ends up in the hands of the xlsx module), so I never actually try to save it to disk. (I did however have success with passing streamand using it with createWriteStream for the purposes of saving the file.)

I was able to recover functionality on my end using the method you landed on too — set responseType to arraybuffer and pass what it returns to Buffer.from. Then I could hand it off to xlsx again!

I'm not sure that node-fetch returns a Buffer when you do res.blob() though — I couldn't get it to validate against Buffer.isBuffer, and I think it instead returns a instance of a custom Blob that's mimicking the browser Blob. But there may be something I'm missing there.

What node-fetch does against a blob:

    /**
     * Return raw response as Blob
     *
     * @return Promise
     */
    blob() {
        let ct = this.headers && this.headers.get('content-type') || '';
        return consumeBody.call(this).then(buf => Object.assign(
            // Prevent copying
            new Blob([], {
                type: ct.toLowerCase()
            }),
            {
                [BUFFER]: buf
            }
        ));
    },

I can't figure out how one is meant to use this Blob implementation in Node. 🤔 It mostly reads like they added it for isomorphic/testing purposes? https://github.com/bitinn/node-fetch/pull/200

[JustinBeckwith]

Yup, you're totally right.

[JustinBeckwith]

Given the current behavior makes a lot more sense, I'm content to close this one out. Please do let me know if there's anything else we could do here!

[rdmurphy]

@JustinBeckwith Sounds good! I agree. Probably best to let fetch continue to work like fetch, it'll not be an issue as this change becomes the understood "default." (I adjusted in my code and it has been fine ever since.) Hopefully folks will find this if they happen to hit it. 👍