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

Wrong stream finish event - when committing workbook stream #198

Closed
junajan opened this issue Oct 17, 2016 · 2 comments
Closed

Wrong stream finish event - when committing workbook stream #198

junajan opened this issue Oct 17, 2016 · 2 comments

Comments

@junajan
Copy link
Contributor

junajan commented Oct 17, 2016

ExcelJs does not finish properly when using stream buffer like this:

@options.workbookOpts = {
#   filename: 'tmp/test.xlsx',
#   stream: @outputStream,
    useStyles: true,
    useSharedStrings: true
};

@workbook = new Excel.stream.xlsx.WorkbookWriter @options.workbookOpts
@worksheet = @workbook.addWorksheet('WSName')
@workbook.stream.pipe(@outputStream)

## add some rows

@workbook.commit()
.then (res) =>
 console.log("END")

On this line https://github.com/guyonroche/exceljs/blob/master/lib/stream/xlsx/workbook-writer.js#L320 you are listening for finish event which is correct for writeStreams (based on node.js doc: https://nodejs.org/api/stream.html#stream_event_finish)

But here in the StreamBuf class you are emitting end event instead:
https://github.com/guyonroche/exceljs/blob/master/lib/utils/stream-buf.js#L287

Possible solutions are:

  1. Emit finish event instead. (Or emit both end and finish events?)
    PR: Fire finish event instead of end event on write stream #199

  2. Don't listen for events on StreamBuf but on zipStream instead.
    PR: Listen for finish event on zip stream instead of middle stream #200

Sideinfo: end event is on readStreams instead.

Workaround: is to use stream: --writeStream-- option instead of piping (in my code it is commented out).

@junajan
Copy link
Contributor Author

junajan commented Oct 17, 2016

I almost forgot... nice work btw

@junajan
Copy link
Contributor Author

junajan commented Nov 4, 2016

Closing as it is already merged to master

@junajan junajan closed this as completed Nov 4, 2016
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

1 participant