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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Streaming I/O Workbook commit never resolve with Stream Passthrough #2200

Open
shujer opened this issue Jan 20, 2023 · 18 comments
Open

Comments

@shujer
Copy link

shujer commented Jan 20, 2023

馃悰 Bug Report

Lib version: 4.3.2

Steps To Reproduce

import * as ExcelJS from 'exceljs'
import { Stream, PassThrough } from 'stream'
const stream = new Stream.Duplex.PassThrough({})
const wb = new ExcelJS.stream.xlsx.WorkbookWriter({
      stream: this.stream,
      useStyles: true,
      useSharedStrings: true
    })
const ws = wb.addWorksheet('XYZ');

ws.addRow({}).commit()
ws.commit()
await wb.commit()

The expected behaviour:

Promise be resolved

Possible solution (optional, but very helpful):

not sure why, any code example for this case? seems finish event not triggered so that commit can't resolved.
Background:
I want to use Stream PassThrough as I can directly to upload the stream to S3 with sdk

@shujer shujer changed the title [BUG] Workbook commit never resolve with Streaming I/O with Stream Passthrough [BUG] Streaming I/O Workbook commit never resolve with Stream Passthrough Jan 21, 2023
@kumaresansv
Copy link

I am also facing similar issue when streaming directly to GCP. Is there a resolution for this? Did you find any workaround?

@kumaresansv
Copy link

I did see a difference in behavior when I updated my default GCP credentials(that got expired) locally. I was not able to create any file on GCP for many of my runs. In all of those runs, the call workbook.commit() never resolved. It almost felt like that the issue was in the workbook.commit() step.
Once I updated my credentials, everything started to work again. I am keeping my fingers crossed for the issue to be associated with expired credentials.

Can you check if you also have similar issue?

@zlooun
Copy link

zlooun commented Mar 3, 2023

So much time has passed, and no one wants to solve such a banal and at the same time important problem. Why waste time on steam if it doesn't work properly anyway?

Solve this problem pls

@HyunTaek5
Copy link

HyunTaek5 commented Mar 7, 2023

I faced similar issue while streaming file directly to AWS S3 bucket. I resolved the issue with removing 'await' in front of workbook commit and make it synchronous. After i removed 'await', I just put Stream into Parameter Body and upload to S3 with aws-sdk.

import * as stream from 'stream';
import * as Excel from 'exceljs';
import { S3 } from 'aws-sdk';

const Stream = new stream.PassThrough();

const wb = new Excel.stream.xlsx.WorkbookWriter({
      stream: Stream,
    });

const ws = wb.addWorksheet('Example');

ws.addRow({}).commit();

ws.commit();
// Remove await in front of workbook commit.
wb.commit();

// Upload Stream to S3
const params = {
      Bucket: ExampleBucket,
      Key: 'example.xlsx',
      Body: stream,
      CacheControl: 'no-cache',
   };

const s3 = new S3({
      region: S3_REGION,
      accessKeyId: ACCESS_KEY,
      secretAccessKey: SECRET_KEY,
});

new Promise<S3.ManagedUpload>((resolve, reject) => {
     s3.upload(params, (err, data) => {
        if (err) reject(err);
        resolve(data);
      });
    });

I don't know why this issue is resolved with removing 'await', but may be there might be some issues with using Stream on Workbook commit?

May be there can be a better way to resolve this issue, but it's out of my league. 馃槀

@zlooun
Copy link

zlooun commented Mar 7, 2023

@HyunTaek5 i solved problem with streams in this fork: https://www.npmjs.com/package/@zlooun/exceljs

@armujahid
Copy link

@zlooun can you please create a PR to resolve this issue in this repo? Because forks will diverge and this issue should be fixed in this repo.

@AnkitKadam567
Copy link

@armujahid @zlooun is this merged? I'm still facing similar issue

@armujahid
Copy link

@AnkitKadam567 for now we are using @zlooun 's fork in production and we are satisfied with its performance and less memory usage using streaming. https://www.npmjs.com/package/@zlooun/exceljs

@AnkitKadam567
Copy link

@armujahid @zlooun I'm using the package suggested by you but still facing same issue

const { stream } = require("@zlooun/exceljs");

function processUpload(bucket, streamData, filename) {
  return new Promise(async (resolve, reject) => {
    const file = await createWritableStream(filename);
    const options = {
      stream: file.createWriteStream(createUploadOptions()),
    };

    const workbook = new stream.xlsx.WorkbookWriter(options);
    const worksheet = workbook.addWorksheet("Sheet1");
    let counter = 0;
    let headersWritten = false;
    streamData.on("data", (data) => {
      counter++;
      console.log("DATA RECEIVED : ", counter);
      if (!headersWritten) {
        worksheet.addRow(Object.keys(data)).commit();
        headersWritten = true;
      }

      worksheet.addRow(Object.values(data)).commit();
    });

    streamData.on("end", async (data) => {
      console.log("COMMITTING WORKBOOK");
      await workbook.commit();
      console.log("RESOLVING PROMISE : ", file.metadata);
      resolve(file.metadata);
    });

    streamData.on("error", async (err) => {
      logger.info("Error while exporting file");
      console.error(err);
      reject(err);
    });
  });
}

am I missing something? I am getting data in stream of JSON format

@zlooun
Copy link

zlooun commented Jul 12, 2023

@AnkitKadam567

It is important to put await before each commit

await worksheet.addRow(Object.keys(data)).commit();

@zlooun
Copy link

zlooun commented Jul 12, 2023

@armujahid
At first I planned it that way, but after I saw that the last time exceljs was published 2 years ago, I decided to make my fork.

@armujahid
Copy link

@zlooun last merge in master was on May 5, 2023. so the repo is active. Maintainer might create a release soon. Or we can create an issue to request a release after merging your PR.

@dmitry-zaporozhan
Copy link

there is active fork https://github.com/zurmokeeper/excelize
maybe they will accept pr

@zurmokeeper
Copy link
Contributor

@dmitry-zaporozhan @armujahid @zlooun @AnkitKadam567 @shujer

Hi everyone, I'm the @zurmokeeper/excelize author, and I'm happy to merge any features or bugfixes that would be useful to you, so feel free to PR.

I used to join the merge meetings of the exceljs maintainers, but nothing has happened since 2023.5.5, so I'm pessimistic about a new version of exceljs, and that's why I feel like deciding to folk a branch out.

@h4l-yup
Copy link

h4l-yup commented Sep 12, 2023

Hi, I am facing the same issue. (workbook.commit never resolved)

Is there no plan for fixing this in this package?

@zlooun
Copy link

zlooun commented Oct 17, 2023

PR: #2558

@baolongt
Copy link

Hi, I am facing the same issue. (workbook.commit never resolved)

Is there no plan for fixing this in this package?

Seems the PR #2558 they mark as release in version 5. Just create patch and wait

@akshayhubilo
Copy link

Its been more than a year, and this issue has still not been resolved. Any timeline?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests