-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
internal: fix TypeError in v8-polyfill #8863
Conversation
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.
LGTM
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'd change the return false
in readline() to return ''
, that way future call sites won't run into the same issue.
If other collaborators think this is fine though, I'm fine with it too.
@bnoordhuis fixed |
This was the failure on AIX https://ci.nodejs.org/job/node-test-commit-aix/1300/
I reran the job and it succeeded: |
PR-URL: #8863 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Landed in 47d1588 |
PR-URL: #8863 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
added lts watch as this lands on v4.x let me know if it should not land |
there's likely no harm in landing it. |
PR-URL: #8863 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #8863 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
v8 profile processor
Description of change
readline()
can return a boolean, in which case, line 90 will throw an exception. This change addresses the issue by defaulting to a string inreadline()
instead of returning false.