Review requested:

• @nodejs/web-infra

@avivkeller what's the latest version of doc-kit atm? Also tests failing 🤔

@avivkeller what's the latest version of doc-kit atm?

1.3.3

Also tests failing 🤔

Flaky tests, unrelated to doc-kit.

@avivkeller what's the latest version of doc-kit atm?

1.3.3

Also tests failing 🤔

Flaky tests, unrelated to doc-kit.

Will this PR automatically get updated to 1.3.3?

Will this PR automatically get updated to 1.3.3?

IIRC @dependabot recreate will recreate a PR that bumps to latest version.

But since I need a bump to fix make test-only on RISC-V, I opened a PR to bump it manually here: #62686

@dependabot recreate

Will this PR automatically get updated to 1.3.3?

IIRC @dependabot recreate will recreate a PR that bumps to latest version.

But since I need a bump to fix make test-only on RISC-V, I opened a PR to bump it manually here: #62686

Didn't notice this was a PR managed by Dependabot, I thought we used our own Bot.

Didn't notice this was a PR managed by Dependabot, I thought we used our own Bot.

We use @nodejs-github-bot when a dependency isn't supported by Dependabot. When we first landed doc-kit, and used git deps, it wasn't supported by Dependabot, now that we release to npm, it is

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

RuntimeError: memory access out of bounds on RHEL / linuxone CI

FYI @nodejs/platform-s390

RuntimeError: memory access out of bounds on RHEL / linuxone CI

There's an s390x related change in nodejs/doc-kit#691 (comment) , would that be related or is it just flaky tests?

This issue seems to be related to Typed array usage under shikijs/engine-oniguruma/dist/index.mjs:
https://github.com/shikijs/shiki/blob/be89afcfb3bfaf8a29c0715bc13b3f95264ce87d/packages/engine-oniguruma/src/oniguruma/index.ts#L293
and
https://github.com/shikijs/shiki/blob/be89afcfb3bfaf8a29c0715bc13b3f95264ce87d/packages/engine-oniguruma/src/oniguruma/index.ts#L360

Wasm is little-endian enforced, Typed arrays follow native-endian order. If you manipulate a Wasm buffer you have to make sure data is also in little-endian format, i.e a fix would look something like this:

    /*const strPtrsPtr = onigBinding.omalloc(4 * patterns.length);
    onigBinding.HEAPU32.set(strPtrsArr, strPtrsPtr / 4);
    const strLenPtr = onigBinding.omalloc(4 * patterns.length);
    onigBinding.HEAPU32.set(strLenArr, strLenPtr / 4);
    */

    const strPtrsPtr = onigBinding.omalloc(4 * patterns.length);
    const strLenPtr = onigBinding.omalloc(4 * patterns.length);
    const dv = new DataView(onigBinding.HEAPU8.buffer);
    for (let i = 0; i < strPtrsArr.length; i++)
      dv.setUint32(strPtrsPtr + i * 4, strPtrsArr[i], true);
    for (let i = 0; i < strLenArr.length; i++)
      dv.setUint32(strLenPtr + i * 4, strLenArr[i], true);

...

  /*const HEAPU32 = onigBinding2.HEAPU32;
  let offset = resultPtr / 4;
  const index = HEAPU32[offset++];
  const count = HEAPU32[offset++];
  */

  const dv2 = new DataView(onigBinding2.HEAPU8.buffer);
  let byteOffset = resultPtr;
  const index = dv2.getUint32(byteOffset, true); byteOffset += 4;
  const count = dv2.getUint32(byteOffset, true); byteOffset += 4;
    const captureIndices = [];
    for (let i = 0; i < count; i++) {
      // const beg = string.convertUtf8OffsetToUtf16(HEAPU32[offset++]);
      // const end = string.convertUtf8OffsetToUtf16(HEAPU32[offset++]);
      const beg = string.convertUtf8OffsetToUtf16(dv2.getUint32(byteOffset, true)); byteOffset += 4;
      const end = string.convertUtf8OffsetToUtf16(dv2.getUint32(byteOffset, true)); byteOffset += 4;

Not sure if this glue code is generated with Emscripten or not, if it is then it needs to be regenerated with SUPPORT_BIG_ENDIAN=1 as explained under this fix: emscripten-core/emscripten#13413

This issue seems to be related to Typed array usage under shikijs/engine-oniguruma/dist/index.mjs: https://github.com/shikijs/shiki/blob/be89afcfb3bfaf8a29c0715bc13b3f95264ce87d/packages/engine-oniguruma/src/oniguruma/index.ts#L293 and https://github.com/shikijs/shiki/blob/be89afcfb3bfaf8a29c0715bc13b3f95264ce87d/packages/engine-oniguruma/src/oniguruma/index.ts#L360

Wasm is little-endian enforced, Typed arrays follow native-endian order. If you manipulate a Wasm buffer you have to make sure data is also in little-endian format, i.e a fix would look something like this:

Thanks for investigating it!

Not sure if this glue code is generated with Emscripten or not, if it is then it needs to be regenerated with SUPPORT_BIG_ENDIAN=1 as explained under this fix: emscripten-core/emscripten#13413

The copyright header in the code tells me that this glue code in shiki is actually copied from https://github.com/microsoft/vscode-oniguruma/blob/main/src/index.ts. vscode-oniguruma indeed uses Emscripten but I cannot find where they generated the index.ts.

But given the complexity and delay involved in fixing code in two upstream repos, perhaps it's better to disable the wasm highlighter for s390x again in doc-kit.

Side note: A few months ago I considered pushing this change to vscode-oniguruma but hesitated since I thought we did a workaround on our side. I should probably have done the PR anyways!

Could anyone request-ci Add this label to start a Jenkins CI on a PR. again? Thanks!

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

It looks like @dependabot recreate didn't bump it to the latest version so the CI is still failing. Not sure why dependabot missed the latest version during the recreate :( .

Frigging hell, Dependabot.

It might be following the cadency of how old a release is. Ill manually override this.

Not sure why dependabot missed the latest version during the recreate :( .

Well, I guess I know what happened. Node.js has configured a dependency cool down here:

So it will take five days before dependabot could bump it to the latest version.

Problem is running npm i on my machine will produce a bigger package-lock diff due to being a different arch and whatnot.

Problem is running npm i on my machine will produce a bigger package-lock diff due to being a different arch and whatnot.

I am preparing a PR to ignore the dependency cooldown for internal packages like doc-kit. The cooldown doesn't make sense for internal packages and will only slow down the propagation of new improvements from doc-kit to node.js repo.

Problem is running npm i on my machine will produce a bigger package-lock diff due to being a different arch and whatnot.

I am preparing a PR to ignore the dependency cooldown for internal packages like doc-kit. The cooldown doesn't make sense for internal packages and will only slow down the propagation of new improvements from doc-kit to node.js repo.

Good point and agreed :)

@dependabot recreate

Landed in dd6a0b5