Skip to content

lib: created isValidCallback validator - #32665

Closed
yashLadha wants to merge 1 commit into
nodejs:masterfrom
yashLadha:refactor/timers_callback
Closed

lib: created isValidCallback validator#32665
yashLadha wants to merge 1 commit into
nodejs:masterfrom
yashLadha:refactor/timers_callback

Conversation

@yashLadha

@yashLadhayashLadha commented Apr 5, 2020

Copy link
Copy Markdown
Contributor

A crucial part of the timers function is to execute the callback function, and
this check is needed by several variants of timer functions and by other modules as well. So to
isolate the checking to a comman function that can be shared by different blocks and thus no need to reimplement the samee thing.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-botnodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Apr 5, 2020

@himself65himself65 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

-1 because we call one more stack now. and not more readable for the coder

@yashLadha

yashLadha commented Apr 5, 2020

Copy link
Copy Markdown
ContributorAuthor

@himself65 One can also say that repeating the code can be moved into one place, following DRY (Don't repeat your code).

Comment threadlib/timers.js Outdated
@rickyes

Copy link
Copy Markdown
Contributor

Maybe it can be moved to lib/internal/validators.js

@yashLadha

yashLadha commented Apr 5, 2020

Copy link
Copy Markdown
ContributorAuthor

@rickyes nice suggestion as this error code is being consumed by other modules as well. I think changing in other modules can be taken in some other PR as not related to the scope of this PR. Will go forward with timers in this PR.

@yashLadha
yashLadhaforce-pushed the refactor/timers_callback branch 2 times, most recently from a9a5337 to 7d4316eCompareApril 5, 2020 12:03
@yashLadhayashLadha changed the title lib: created isValidCallback helperlib: created isValidCallback validatorApr 5, 2020
check for callback function is moved to a separate function.
This piece of code is being shared by other entities as well.
@yashLadha
yashLadhaforce-pushed the refactor/timers_callback branch from 7d4316e to 7ef2f2aCompareApril 5, 2020 17:30
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

himself65 pushed a commit that referenced this pull request Apr 14, 2020
check for callback function is moved to a separate function.
This piece of code is being shared by other entities as well.
PR-URL: #32665
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
@himself65

Copy link
Copy Markdown
Member

Landed in 55b4d03

BethGriggs pushed a commit that referenced this pull request Apr 14, 2020
check for callback function is moved to a separate function.
This piece of code is being shared by other entities as well.
PR-URL: #32665
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
check for callback function is moved to a separate function.
This piece of code is being shared by other entities as well.
PR-URL: nodejs#32665
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
check for callback function is moved to a separate function.
This piece of code is being shared by other entities as well.
PR-URL: #32665
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
@BridgeARBridgeAR mentioned this pull request Apr 28, 2020
targos pushed a commit that referenced this pull request Apr 28, 2020
check for callback function is moved to a separate function.
This piece of code is being shared by other entities as well.
PR-URL: #32665
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
@targostargos mentioned this pull request May 2, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

timersIssues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@yashLadha@rickyes@nodejs-github-bot@himself65@jasnell@addaleax@Flarna