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

Drive API: example mistake #1788

Closed
ttv20 opened this issue Aug 8, 2019 · 2 comments · Fixed by #1798
Closed

Drive API: example mistake #1788

ttv20 opened this issue Aug 8, 2019 · 2 comments · Fixed by #1798
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. type: docs Improvement to the documentation for an API.

Comments

@ttv20
Copy link
Contributor

ttv20 commented Aug 8, 2019

On /samples/drive/export.js there is a mistake
I tried the function a few times and it downloaded 20kb less each time
So I discover that the function finish to run by await to res.data.on('end')
but the stream didn't finish to write the data to the fs yet
The correct function is:

async function runSample() {
  // [START main_body]
  const fileId = '1EkgdLY3T-_9hWml0VssdDWQZLEc8qqpMB77Nvsx6khA';
  const destPath = path.join(os.tmpdir(), 'important.pdf');
  const dest = fs.createWriteStream(destPath);
  const res = await drive.files.export(
    {fileId, mimeType: 'application/pdf'},
    {responseType: 'stream'}
  );
  res.data.pipe(dest)
  await new Promise((resolve, reject) => {
    dest
      .on('finish', () => {
        console.log(`Done downloading document: ${destPath}.`);
        resolve();
      })
      .on('error', err => {
        console.error('Error downloading document.');
        reject(err);
      })
  });
  // [END main_body]
}
@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Aug 9, 2019
@ttv20 ttv20 changed the title example mistake Drive API: example mistake Aug 12, 2019
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Aug 14, 2019
@JustinBeckwith JustinBeckwith 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 Aug 15, 2019
@yoshi-automation yoshi-automation removed 🚨 This issue needs some love. triage me I really want to be triaged. labels Aug 15, 2019
@ttv20
Copy link
Contributor Author

ttv20 commented Aug 19, 2019

After I learn how to use stream:
The problem with the example is that the function wait for the end event of the read stream instead of waiting for the finish event of the file write stream to finish
Better code with error handling for the download and the file stream:

async function runSample() {
  // [START main_body]
  const fileId = '1EkgdLY3T-_9hWml0VssdDWQZLEc8qqpMB77Nvsx6khA';
  const destPath = path.join(os.tmpdir(), 'important.pdf');
  const dest = fs.createWriteStream(destPath);
  const res = await drive.files.export(
    {fileId, mimeType: 'application/pdf'},
    {responseType: 'stream'}
  );
await Promise.all([
    new Promise((resolve, reject) => {
      res.data
        .on('end', () => {
          console.log(`Done downloading document: ${destPath}.`);
          resolve();
        })
        .on('error', err => {
          console.error('Error downloading document.');
          reject(err);
        })
        .pipe(dest);
    }),
    new Promise((resolve, reject) => {
      dest
        .on('finish', () => {
          console.log(`Done saving document: ${destPath}.`);
          resolve();
        })
        .on('error', err => {
          console.error('Error saving document.');
          reject(err);
        })
    })
  ])
  // [END main_body]
}

@bcoe bcoe added the type: docs Improvement to the documentation for an API. label Aug 20, 2019
@bcoe
Copy link
Contributor

bcoe commented Aug 20, 2019

@ttv20 good catch 👍 would happily land an update to the sample with end changed to finish. I'll leave this open, in case you don't have time to contribute the patch, and we'll update the sample soon.

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. type: docs Improvement to the documentation for an API.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants