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

Fix incorrect Object/function checking in node index.js wrapper #3177

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kudlav
Copy link
Contributor

@kudlav kudlav commented Jan 24, 2025

Node
Fix incorrect Object/function checking in node index.js wrapper.

Changes

  • replace options instanceof Object by typeof options === "object"
  • replace options.request instanceof Function by typeof options.request === "function"

Using instanceof Object can be problematic because it won't work correctly for all objects.
E.g. the following object { ratio: 1, request: [Function: request] } returns options instanceof Object false and at the same time typeof options.request === "function" true. Simillar to the function check.

Other changes

  • Use modern let and const instead of var to prevent hoisting issues
  • Unify single and double quote to double quote - use just one type in a single file
  • do not repeat if (options... every time, use nested if

@louwers louwers added the node label Jan 24, 2025
@louwers louwers requested a review from acalcutt February 6, 2025 20:42
Copy link
Collaborator

@louwers louwers left a comment

Choose a reason for hiding this comment

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

Looks good, the Node.js codebase can be modernized a bit. I would recommend setting up an automated formatter, maybe with pre-commit. Could you add some regression tests for the scenarios you describe?

Maybe we can create a Node.js channel on Slack to coordinate the continued development of the Node.js platform?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants