-
-
Notifications
You must be signed in to change notification settings - Fork 8
refactor: use for loop replace spread to fix stack overflow #15
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
Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
dev/lib/edit-map.js
Outdated
| while (slice) { | ||
| events.push(...slice) | ||
| // NOTE: do not use spread operator here, it will cause a stack overflow. | ||
| for (let i = 0; i < slice.length; i++) { |
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.
Adding a loop could could significant negative performance impact.
Could concat be used instead?
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 try to use Array.contact to replace for loop, but the event is a parameter, I just can modify it in-place, do you have any recommend, thx.
dev/lib/edit-map.js
Outdated
|
|
||
| while (slice) { | ||
| events.push(...slice) | ||
| // NOTE: do not use spread operator here, it will cause a stack overflow. |
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.
Better than a comment, would be a test that fails with the current logic and passes after
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 add a large markdown file to test/fixtures, it will fail when run the original codes.
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.
Thanks, could we remove this comment now, since the test will cover it?
82a2173 to
13a0754
Compare
dev/lib/edit-map.js
Outdated
|
|
||
| while (slice) { | ||
| events.push(...slice) | ||
| // NOTE: do not use spread operator here, it will cause a stack overflow. |
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.
Thanks, could we remove this comment now, since the test will cover it?
|
sure, I have removed unnecessary comment, thank you for the review and your time |
This comment has been minimized.
This comment has been minimized.
|
thanks, released in |
Initial checklist
Description of changes
Main change is use
forreplace w/ spread opreator[...slice]When I try to parse a large markdown file, about 1.6 Mb, I meet an issue
RangeError: Maximum call stack size exceededI debugged the source code and found that the error came from the
sliceline. I suspected that the spread operator caused the stack overflow.After, change it to
for, the issue has fixed.