- Notifications
You must be signed in to change notification settings - Fork1.9k
Optimize SAXStream#1140
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Optimize SAXStream#1140
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Could we try to review/merge this PR? If anything here is unclear, is there anything I can improve? |
@alubbe I'm going to review when I will have some time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Generally, it looks ok :) I have only one quite simple note here.
Let's take me a minute before merging to test it withmy lib xlsx-renderer. There it has some testes which should additionally check if reader and writer work 100% correctly.
| super(); | ||
| this.sax=newsaxes.SaxesParser(); | ||
| module.exports=classSAXStreamextendsStream.Transform{ | ||
| constructor(eventTypes){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I see thateventTypes everywhere has the same value, assax-stream.js is an internal utility I think you should use default value here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good catch - fixed in#1135
|
This PR brings over improvements and speed-ups from#1135 that work on nodejs v8. It will help make the diff of the other PR smaller