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

Minor fixes for stream handling #267

Merged
merged 2 commits into from
Mar 10, 2017
Merged

Conversation

holm
Copy link
Contributor

@holm holm commented Feb 20, 2017

I upgraded from 0.2.38 to 0.2.46 today, and a few of our testcases failed. I tracked it down to two issues:

  • The catch handler on the zip parsing was missing, and also wasn't propagating the error via the event emitter.
  • The workbook writer was resolving the write when the zip-stream was done, not when the underlying stream was done. This meant that the write could resolve, while f.ex. a file was still being written to disk.

@neuralp
Copy link

neuralp commented Feb 23, 2017

Thank you for this simple fix. I was just trying to figure out why my catch wasn't working while passing bad files to the readFile function and behold, you have the answer!

@guyonroche
Copy link
Collaborator

@holm - you wouldn't be able to add one of those failing test cases (or a smaller version of one) would you?
This (or something related to it) has been a recurring issue and it would be good to prevent it from happening again.

@holm
Copy link
Contributor Author

holm commented Feb 23, 2017

I can try to find the time next week. I just needed it fixed quite urgently for one of our customers, but I should have time soon.

@guyonroche guyonroche merged commit 17f24b6 into exceljs:master Mar 10, 2017
@guyonroche
Copy link
Collaborator

@holm - Just looking into this now, I hope to publish this weekend. I just noticed the change in zip-stream removed the .catch() on entry.async(), was that necessary or is it just that entry.async() doesn't reject?

@holm
Copy link
Contributor Author

holm commented Mar 10, 2017

Yeah, that looks like a mistake. Should most likely emit the error there also.

@guyonroche
Copy link
Collaborator

@holm Cool - thanks.
I've restored the other catch.
All tests passing with this and the updated lib versions so I've just published => 0.3.1.

@holm
Copy link
Contributor Author

holm commented Mar 10, 2017

Thanks a lot for all your work on this library, it's pretty great!

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