Skip to content

lib: check SharedArrayBuffer existence in fast-utf8-stream#60453

Closed
codebytere wants to merge 1 commit into
nodejs:mainfrom
codebytere:sab-undefined-utf8stream
Closed

lib: check SharedArrayBuffer existence in fast-utf8-stream#60453
codebytere wants to merge 1 commit into
nodejs:mainfrom
codebytere:sab-undefined-utf8stream

Conversation

@codebytere

Copy link
Copy Markdown
Member

Refs #58897

Fixes the following error:

"Uncaught ReferenceError: SharedArrayBuffer is not defined"

caused by changes in the above PR that declare const kNil = new Int32Array(new SharedArrayBuffer(4)); when lib/internal/streams/fast-utf8-stream.js is imported and Utf8Stream() is used. This causes an error in any environment missing a SharedArrayBuffer.

I used an alternative sleep implementation in the missing case, but am happy to rework this however is preferred.

@codebytere codebytere requested a review from jasnell October 28, 2025 08:04
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem. labels Oct 28, 2025
Comment on lines +52 to +53
const haveSAB = typeof SharedArrayBuffer !== 'undefined';
const kNil = haveSAB ? new Int32Array(new SharedArrayBuffer(4)) : null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is the haveSAB variable useful?

Suggested change
const haveSAB = typeof SharedArrayBuffer !== 'undefined';
const kNil = haveSAB ? new Int32Array(new SharedArrayBuffer(4)) : null;
const kNil = SharedArrayBuffer && new Int32Array(new SharedArrayBuffer(4));

@codebytere codebytere Oct 28, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@aduh95 we need it below to choose sleep implementation, no?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could use kNil for that

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The suggested code would throw an error if SharedArrayBuffer is not defined.

@aduh95 aduh95 Oct 28, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is declared above, it wouldn't throw even if undefined:

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@codecov

codecov Bot commented Oct 28, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 37.50000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.54%. Comparing base (ddbe136) to head (38de540).
⚠️ Report is 75 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/streams/fast-utf8-stream.js 37.50% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #60453      +/-   ##
==========================================
- Coverage   88.59%   88.54%   -0.05%     
==========================================
  Files         704      704              
  Lines      207774   207848      +74     
  Branches    40035    40045      +10     
==========================================
- Hits       184068   184036      -32     
- Misses      15748    15850     +102     
- Partials     7958     7962       +4     
Files with missing lines Coverage Δ
lib/internal/streams/fast-utf8-stream.js 73.80% <37.50%> (-0.41%) ⬇️

... and 66 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@codebytere codebytere force-pushed the sab-undefined-utf8stream branch from 6547f21 to 38de540 Compare November 2, 2025 15:55
@codebytere codebytere requested review from aduh95 and targos November 2, 2025 15:55
@codebytere codebytere changed the title lib,streams: check SharedArrayBuffer existence in fast-utf8-stream lib: check SharedArrayBuffer existence in fast-utf8-stream Nov 2, 2025
@codebytere codebytere requested a review from cjihrig November 3, 2025 09:58
@codebytere codebytere added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 3, 2025
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 3, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@codebytere codebytere closed this Nov 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants