profile
viewpoint

nodejs/corepack 1316

Zero-runtime-dependency package acting as bridge between Node projects and their package managers

nodejs/citgm 501

Canary in the Gold Mine

nodejs/build 452

Better build and test infra for Node.

nodejs/community-committee 263

The Node.js Community Committee (aka CommComm)

nodejs/abi-stable-node-addon-examples 240

Node Add-on Examples with PoC ABI stable API for native modules

nodejs/abi-stable-node 235

Repository used by the Node-API team to manage work related to Node-API and node-addon-api

nodejs/changelog-maker 214

A git log to CHANGELOG.md tool

nodejs/cjs-module-lexer 178

Fast lexer to extract named exports via analysis from CommonJS modules

nodejs/code-and-learn 163

A series of workshop sprints for Node.js.

nodejs/benchmarking 160

Node.js Benchmarking Working Group

issue commentnodejs/build

Access to ubuntu2004-arm64

@ShogunPanda if you send me your ssh key I'll add it to the host and give you instructions how to get into the machine.

ShogunPanda

comment created time in 4 hours

issue commentnodejs/loaders

Node.js Loaders Team Meeting 2023-03-28

I’m not sure what decision could need to be made? If you find a solution for the console.log issues then I think just make a PR, and if there are any tradeoffs involved they can be analyzed in the context of landing that PR.

mhdawson

comment created time in 4 hours

Pull request review commentnodejs/node

test_runner: add code coverage support to spec reporter

 function getCoverageFixtureReport() {   return report; } -test('--experimental-test-coverage and --test cannot be combined', () => {-  // TODO(cjihrig): This test can be removed once multi-process code coverage-  // is supported.-  const args = ['--test', '--experimental-test-coverage'];-  const result = spawnSync(process.execPath, args);--  // 9 is the documented exit code for an invalid CLI argument.-  assert.strictEqual(result.status, 9);-  assert.match(-    result.stderr.toString(),-    /--experimental-test-coverage cannot be used with --test/-  );-});+function getSpecCoverageFixtureReport() {+  const report = [+    '\u2139 start of coverage report',+    '\u2139 file | line % | branch % | funcs % | uncovered lines',+    '\u2139 test/fixtures/test-runner/coverage.js | 78.65 | 38.46 | 60.00 | 12, ' ++    '13, 16, 17, 18, 19, 20, 21, 22, 27, 39, 43, 44, 61, 62, 66, 67, 71, 72',+    '\u2139 test/fixtures/test-runner/invalid-tap.js | 100.00 | 100.00 | 100.00 | ',+    '\u2139 test/fixtures/v8-coverage/throw.js | 71.43 | 50.00 | 100.00 | 5, 6',+    '\u2139 all files | 78.35 | 43.75 | 60.00 |',+    '\u2139 end of coverage report',+  ].join('\n'); -test('handles the inspector not being available', (t) => {-  if (process.features.inspector) {-    return;+  if (common.isWindows) {+    return report.replaceAll('/', '\\');   } -  const fixture = fixtures.path('test-runner', 'coverage.js');-  const args = ['--experimental-test-coverage', fixture];-  const result = spawnSync(process.execPath, args);+  return report;+} -  assert(!result.stdout.toString().includes('# start of coverage report'));-  assert(result.stderr.toString().includes('coverage could not be collected'));-  assert.strictEqual(result.status, 0);-  assert(!findCoverageFileForPid(result.pid));-});+test('test coverage report', async (t) => {+  await t.test('--experimental-test-coverage and --test cannot be combined', () => {+    // TODO(cjihrig): This test can be removed once multi-process code coverage+    // is supported.+    const args = ['--test', '--experimental-test-coverage'];+    const result = spawnSync(process.execPath, args);++    // 9 is the documented exit code for an invalid CLI argument.+    assert.strictEqual(result.status, 9);+    assert.match(+      result.stderr.toString(),+      /--experimental-test-coverage cannot be used with --test/+    );+  }); -test('coverage is reported and dumped to NODE_V8_COVERAGE if present', (t) => {-  if (!process.features.inspector) {-    return;-  }+  await t.test('handles the inspector not being available', (t) => {+    if (process.features.inspector) {+      return;+    } -  const fixture = fixtures.path('test-runner', 'coverage.js');-  const args = ['--experimental-test-coverage', fixture];-  const options = { env: { ...process.env, NODE_V8_COVERAGE: tmpdir.path } };-  const result = spawnSync(process.execPath, args, options);-  const report = getCoverageFixtureReport();+    const fixture = fixtures.path('test-runner', 'coverage.js');+    const args = ['--experimental-test-coverage', fixture];+    const result = spawnSync(process.execPath, args); -  assert(result.stdout.toString().includes(report));-  assert.strictEqual(result.stderr.toString(), '');-  assert.strictEqual(result.status, 0);-  assert(findCoverageFileForPid(result.pid));+    assert(!result.stdout.toString().includes('# start of coverage report'));+    assert(result.stderr.toString().includes('coverage could not be collected'));+    assert.strictEqual(result.status, 0);+    assert(!findCoverageFileForPid(result.pid));+  }); }); -test('coverage is reported without NODE_V8_COVERAGE present', (t) => {-  if (!process.features.inspector) {-    return;-  }+test('test tap coverage reporter', async (t) => {

we can do in this pr 👍🏼

pulkit-30

comment created time in 4 hours

PullRequestReviewEvent

pull request commentnodejs/node

quic: implement various utilities classes to be used by the quic impl

CI: https://ci.nodejs.org/job/node-test-pull-request/50648/

jasnell

comment created time in 4 hours

issue openednodejs/node

Support listenSync() on net, http module

What is the problem this feature will solve?

I'm currently in the process of putting together a Node and Deno networking library, but am running into some problems trying to align the runtime behavior around listen due to Node only supporting asynchronous (and non try/catch-able) network interface binding.

Node

import * as net from 'node:net'

try {
  const listener1 = new net.Server()
  listener1.listen(5000)
  const listener2 = new net.Server()
  listener2.listen(5000)
} catch (error) {
  console.log('unable to catch the error synchronously')
}

Deno

try {
  const listener1 = Deno.listen({ port: 5000, transport: 'tcp' })
  const listener2 = Deno.listen({ port: 5000, transport: 'tcp' })
} catch(error) {
  console.log('caught the error synchronously')
}

What is the feature you are proposing to solve the problem?

This feature request to suggest adding a possible listenSync() function to allow for synchronous listening and enable EADDRINUSE type errors to be caught via try/catch

Node

import * as net from 'node:net'

try {
  const listener1 = new net.Server()
  listener1.listenSync(5000)
  const listener2 = new net.Server()
  listener2.listenSync(5000)
} catch (error) {
  console.log('caught the error synchronously')
}

What alternatives have you considered?

I've tried a few things (including having Deno artificially return a listener via a Promise) but have settled on the following high level design (which unfortunately has both platforms throw at different locations)

Node

import { Net } from 'mylib'

const listener = Net.listen({ port: 5000 }) // Deno will throw here.

for await(const socket of listener) {       // Node will throw here (on first read)
}

I tend to think the Deno synchronous listen is quite intuitive and does help catch EADDRINUSE type errors early (which have historically been quite challenging to deal with in Node due to latent errors coming through on next tick). It would be great to see a similar listenSync() available both for the node:net, node:http and core modules.

created time in 4 hours

startednodejs/docker-node

started time in 4 hours

issue commentnodejs/loaders

Node.js Loaders Team Meeting 2023-03-28

I can meet. Do we need to make a decision (that should be recorded) or just pair/brainstorm (which could just be a huddle)?

mhdawson

comment created time in 4 hours

pull request commentnodejs/node

v16.20.0 proposal

Maybe we should include #47256

Even more reason not to rush, 2023b is now invalidated due to Lebanon withdrawing the change it was created to address, and users are recommended to either stick with 2023a (which we never landed in Node.js) or wait for 2023c.

Refs: https://mm.icann.org/pipermail/tz/2023-March/032829.html Refs: https://mm.icann.org/pipermail/tz/2023-March/032833.html

BethGriggs

comment created time in 4 hours

pull request commentnodejs/node

deps: update timezone to 2023b

cc @nodejs/releasers So it seems that 2023b was for "Lebanon delays the start of DST this year": https://mm.icann.org/pipermail/tz-announce/2023-March/000078.html However it sounds like this decision has been reversed, (there's a lot of chatter on https://mm.icann.org/pipermail/tz/2023-March/thread.html) and for now users should either stick to 2023a or wait for 2023c. https://mm.icann.org/pipermail/tz/2023-March/032833.html

nodejs-github-bot

comment created time in 4 hours

PullRequestReviewEvent

Pull request review commentnodejs/node

meta: clarify the threat model to explain the JSON.parse case

 lead to a loss of confidentiality, integrity, or availability.    npm registry.    The code run inherits all the privileges of the execution user. 4. Inputs provided to it by the code it is asked to run, as it is the-   responsibility of the application to perform the required input validations.+   responsibility of the application to perform the required input validations,+   e.g. the input to `JSON.parse()`.
   e.g., the input to `JSON.parse()`.
mcollina

comment created time in 6 hours

PullRequestReviewEvent

issue commentnodejs/loaders

Node.js Loaders Team Meeting 2023-03-28

I’m not sure I can attend today, but if you’d like to meet please feel free. @aduh95 or @JakobJingleheimer I assume either of you can run the Zoom/YouTube?

mhdawson

comment created time in 4 hours

issue commentnodejs/node

Add a promise-based API and a synchronous API to the stream module

There are various ways to use streams in a way that is compatible with Promises. Support for async iterables is great, and you can also construct your own async iterators using events.on, for example. pipeline() and finished() also work nicely with Promises.

As @aduh95 mentioned, toArray() returns a Promise, and there are also forEach(), map(), etc. to solve common tasks.

If you want an API that is built on top of Promises, you can also use the Web Streams API.

All that being said, you are reading an entire stream and then pass the buffered contents to JSON.parse(). Doing so eliminates virtually all benefits of streams. Streams reduce latency and memory requirements only if the data is processed chunk by chunk, but passing it to JSON.parse() means that it is processed all at once. Why do you use a stream in the first place instead of a string or a Promise<string>? Either can be obtained easily in a sync or async fashion, and it can also be mocked trivially for testing.

geryogam

comment created time in 4 hours

pull request commentnodejs/node

v16.20.0 proposal

Maybe we should include #47256

We'd either need to manually patch the timezone db for ICU 71.1 or update Node.js 16 to ICU 72.1 (which I'm a little hesitant about because we had issues raised about ICU 72.1 around the change to Unicode spaces -- although I think there was a V8 patch to revert (which we'd also then need to avoid breakage)).

BethGriggs

comment created time in 5 hours

issue commentnodejs/loaders

Node.js Loaders Team Meeting 2023-03-28

I’m not sure why nodejs/node#30491 is on the agenda.

Because console.log flakiness inside worker threads is an issue for loaders (and has been a pain for our own tests). If we have time, we could discuss what solutions we could come up with.

mhdawson

comment created time in 5 hours

startednodejs/node

started time in 5 hours

startednodejs/node

started time in 5 hours

PullRequestReviewEvent

fork tripplicate/llhttp

Port of http_parser to llparse

http://llhttp.org

fork in 5 hours

PullRequestReviewEvent
PullRequestReviewEvent

startednodejs/node

started time in 5 hours

issue commentnodejs/diagnostics

TC39 proposal AsyncContext

Good to see that progressing in TC39.

legendecas

comment created time in 5 hours

PR opened nodejs/build

Add Windows ARM64 assets to release

This change brings Windows ARM64 one step closer to being a tier 2 supported platform, as described in https://github.com/nodejs/node/pull/47233.

With these changes, Windows ARM64 assets will get picked when releasing the following versions of NodeJS. Please note that these changes are partial, the rest of the required changes (eg. modifying the release job) will be done by @joaocgreis.

+10 -2

0 comment

3 changed files

pr created time in 5 hours

pull request commentnodejs/node

vm: fix leak in vm.compileFunction when importModuleDynamically is used

Since this is reverted on 19.8.1 is there a follow up issue I can track for a follow up diff that isn't crashy?

joyeecheung

comment created time in 6 hours

PullRequestReviewEvent

pull request commentnodejs/node

v16.20.0 proposal

Maybe we should include https://github.com/nodejs/node/pull/47256

BethGriggs

comment created time in 6 hours

startednodejs/abi-stable-node-addon-examples

started time in 6 hours

more