Skip to content

Conversation

@vgrem
Copy link

@vgrem vgrem commented May 1, 2018

Introduce a support to download content as a binary

.getBinary

Use .getBinary() to stream a download from Microsoft Graph.

const fs = require('fs'); // requires filesystem module
client
    .api('/me/drive/root/children/Book.docx/content') // path of  source file in OneDrive
    .getBinary((err, buffer) => {
           fs.writeFileSync('Book.docx', buffer,'binary');
    });

@msftclas
Copy link

msftclas commented May 1, 2018

CLA assistant check
All CLA requirements met.

@MIchaelMainer
Copy link
Contributor

MIchaelMainer commented May 1, 2018

Thank you for the pull request! This will be helpful. I'm sorry but I can't get to this right now. We still need to finish the update to use the Fetch API instead of superagent/xhr. 765594

@muthurathinam
Copy link
Contributor

muthurathinam commented Aug 6, 2018

@vgrem @MIchaelMainer @darrelmiller
This can be achieved by setting responseType to the get call.

client
    .api(`/me/drive/root/children/${fileName}/content`)
    .responseType("blob")
    .get()
    .then((res) => {
        console.log("done");
        console.log(res);
    })
    .catch((err) => {
        throw err;
    });

Do we really need this api change ? -> https://github.com/microsoftgraph/msgraph-sdk-javascript/pull/70/files#diff-28bf2624bd9644553428f50cb8aac3d7R300

@MIchaelMainer
Copy link
Contributor

MIchaelMainer commented Aug 6, 2018

We don't need it.

Is it discoverable that someone should needs to set the responseType to "blob"? Do we have docs on it. If it isn't, then getBinary may make more sense. We also have a getStream, does that do the same thing?

The easiest thing we can do is update the readme to make it clear how we get binary data.

@muthurathinam
Copy link
Contributor

@MIchaelMainer I agree it is not discoverable as of now. As you mentioned we can update the readme about how to get binary data.
As I am concerned about the size of the output js file, adding such APIs those are achievable in other ways might be unnecessary.
I feel same with the stream APIs, we should have added the guide to achieve that instead of providing new API.

@muthurathinam
Copy link
Contributor

@vgrem @MIchaelMainer

Changes merged with this PR, So closing this one.!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants