Skip to content

Fix flushing rows in the end, correctly wait for drain event, restructure streams.#16

Merged
jirimoravcik merged 1 commit intomasterfrom
fix/row-transform-flushing
Jul 22, 2021
Merged

Fix flushing rows in the end, correctly wait for drain event, restructure streams.#16
jirimoravcik merged 1 commit intomasterfrom
fix/row-transform-flushing

Conversation

@jirimoravcik
Copy link
Copy Markdown
Member

This PR fixes an issue, where in some cases, the row transform stream would not transform all rows and the XLSX file would be incomplete. Also can be considered a refactor since the code got simplified (no need to use a PassThrough stream anymore).

  • Remove PassThrough stream and add sheet header and footer directly in the RowTransform class
  • Correctly wait for drain event when pushing to RowTransform to prevent memory buffering. (based off Node.js docs https://nodejs.org/api/stream.html#stream_writable_write_chunk_encoding_callback)
  • Update unit tests to reflect the new RowTransform behavior and add a larger high level test (currently a file with 500 rows - that's a size on which I was able to observe the flushing issue)

Copy link
Copy Markdown
Member

@mtrunkat mtrunkat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a bit of magic :)

@jirimoravcik jirimoravcik merged commit b660f6f into master Jul 22, 2021
@jirimoravcik jirimoravcik deleted the fix/row-transform-flushing branch May 13, 2024 10:03
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

Successfully merging this pull request may close these issues.

3 participants