Skip to content

Conversation

@solebox
Copy link
Contributor

@soleboxsolebox commented Oct 31, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

vm

Description of change

named arrow function handlers :)

Ref: #8913

@nodejs-github-botnodejs-github-bot added the vm Issues and PRs related to the vm subsystem. label Oct 31, 2016
lib/vm.js Outdated
Script.prototype.runInThisContext=function(options){
if(options&&options.breakOnSigint){
returnsigintHandlersWrap(()=>{
constsigintHandler=()=>{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If line length is below 80 chars I think it is ok to put the whole function expression on one line.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit 😕 about the name here… this function does not handle any kind of signal; it’s the exact opposite, the function is run undisturbed by SIGINTs. (The comment/code for sigintHandlersWrap has a bit more information.) So maybe you could call it realRunInThisContext or runScriptUninterruped or something like that?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is the doc you are referring to , i want to understand this a little better

@benjamingr
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-commit/5938/console

Going to land if no one objects.

benjamingr pushed a commit that referenced this pull request Nov 6, 2016
Name anonymous arrow function in vm module to improve readability PR-URL: #9388 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Ref: #8913
@benjamingr
Copy link
Member

Landed in 5079763

Thanks!

evanlucas pushed a commit that referenced this pull request Nov 7, 2016
Name anonymous arrow function in vm module to improve readability PR-URL: #9388 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Ref: #8913
@evanlucasevanlucas mentioned this pull request Nov 7, 2016
@MylesBorinsMylesBorins added the baking-for-lts PRs that need to wait before landing in a LTS release. label May 8, 2017
@TrottTrott mentioned this pull request May 15, 2017
3 tasks
@gibfahn
Copy link
Member

#10816 should land at the same time as this.

@MylesBorinsMylesBorins added dont-land-on-v6.x and removed baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Nov 14, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

vmIssues and PRs related to the vm subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@solebox@benjamingr@gibfahn@jasnell@addaleax@lpinca@MylesBorins@nodejs-github-bot