-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: changed var to const in internal/v8_polyfill #8615
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
@matthewloring is this copied verbatim from in V8? If so we may not want to do this? |
@Fishrock123 I thought about that but since it is a polyfill and there is no comment about it I went ahead. If this is a copied verbatim we might want to add a comment in the file about that. |
These are not copied from V8. LGTM. |
why is there a "Copyright 2012 the V8 project authors." then ? |
The 2012 is an error. I believe I originally did the work in V8 and then moved it over to Node.js. Let me look back and double check. |
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
PR-URL: #8615 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Landed in 99f64f7. Thank you! |
PR-URL: #8615 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #8615 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Checklist
make -j4 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
internal
Description of change
Changed
var
toconst
.Part of nodejs/code-and-learn#56