Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Update to Node.js 18.19.0, add Node 21.x to CI#528

Merged
mcollina merged 8 commits intomainnodejs/readable-stream:mainfrom
update-to-node-v18.19.0nodejs/readable-stream:update-to-node-v18.19.0Copy head branch name to clipboard
Dec 18, 2023
Merged

Update to Node.js 18.19.0, add Node 21.x to CI#528
mcollina merged 8 commits intomainnodejs/readable-stream:mainfrom
update-to-node-v18.19.0nodejs/readable-stream:update-to-node-v18.19.0Copy head branch name to clipboard

Conversation

@mcollina
Copy link
Member

No description provided.

Signed-off-by: Matteo Collina <hello@matteocollina.com>
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

Signed-off-by: Matteo Collina <hello@matteocollina.com>
@mcollina
Copy link
Member Author

@atlowChemi I need some help here in providing a polyfill for AbortSignal.any for Node v12 ,v14 and v16.

Signed-off-by: Matteo Collina <hello@matteocollina.com>
@mcollina
Copy link
Member Author

@atlowChemi I implemented something, PTAL

Signed-off-by: Matteo Collina <hello@matteocollina.com>
Signed-off-by: Matteo Collina <hello@matteocollina.com>
Signed-off-by: Matteo Collina <hello@matteocollina.com>
@mcollina
Copy link
Member Author

All green

@mcollina mcollina requested a review from vweevers December 15, 2023 12:33
lib/ours/util.js Outdated
Comment on lines +139 to +140
// validateAbortSignal(signal, 'signal');
// validateFunction(listener, 'listener');
Copy link
Member

Choose a reason for hiding this comment

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

Any specific reason to not implement these?

Copy link
Member Author

Choose a reason for hiding this comment

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

No particular reasons. Do you know where I can copy them from?

Copy link
Member Author

Choose a reason for hiding this comment

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

The biggest reason I didn't test them is that they were not needed for the tests to pass.

Copy link
Member

Choose a reason for hiding this comment

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

They might not be required for the tests 🤷🏽‍♂️
https://github.com/nodejs/node/blob/01719333623aaa324d05011ffcba0dddc0ea3666/lib/internal/validators.js#L421-L428
https://github.com/nodejs/node/blob/01719333623aaa324d05011ffcba0dddc0ea3666/lib/internal/validators.js#L438-L441

const validateAbortSignal = (signal, name) => {
  if (signal !== undefined &&
      (signal === null ||
       typeof signal !== 'object' ||
       !('aborted' in signal))) {
    throw new ERR_INVALID_ARG_TYPE(name, 'AbortSignal', signal);
  }
};
const validateFunction = (value, name) => {
  if (typeof value !== 'function')
    throw new ERR_INVALID_ARG_TYPE(name, 'Function', value);
};

lib/ours/util.js Show resolved Hide resolved
src/util.js Show resolved Hide resolved
Signed-off-by: Matteo Collina <hello@matteocollina.com>
@mcollina
Copy link
Member Author

cc @atlowChemi

Signed-off-by: Matteo Collina <hello@matteocollina.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.