Skip to content
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

lib: created isValidCallback validator #32665

Closed
wants to merge 1 commit into from

Conversation

yashLadha
Copy link
Member

@yashLadha yashLadha commented Apr 5, 2020

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-bot nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Apr 5, 2020
Copy link
Member

@himself65 himself65 left a comment

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
Copy link
Member Author

yashLadha commented Apr 5, 2020

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

lib/timers.js Outdated Show resolved Hide resolved
@rickyes
Copy link
Contributor

rickyes commented Apr 5, 2020

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

@yashLadha
Copy link
Member Author

yashLadha commented Apr 5, 2020

@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 yashLadha force-pushed the refactor/timers_callback branch 2 times, most recently from a9a5337 to 7d4316e Compare April 5, 2020 12:03
@yashLadha yashLadha changed the title lib: created isValidCallback helper lib: created isValidCallback validator Apr 5, 2020
check for callback function is moved to a separate function.
This piece of code is being shared by other entities as well.
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
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
Member

Landed in 55b4d03

@himself65 himself65 closed this Apr 14, 2020
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>
@BridgeAR BridgeAR 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants