lib: fix TypeError when converting a detached buffer source by cola119 · Pull Request #44020 · nodejs/node · GitHub

Currently TextDecoder.decode will throw TypeError when a detached buffer is given since it will try to convert a detached buffer into new buffer. This PR fixed TypeError by checking if a buffer is detached.

const buffer = new ArrayBuffer(1);
new MessageChannel().port1.postMessage(buffer, [buffer]); // buffer is detached
const decoder = new TextDecoder();
decoder.decode(buffer);
// node: TypeError: Cannot perform Construct on a detached ArrayBuffer
// chrome/safari/deno: ""

As a user, I would rather have the error than an empty buffer. The reason is that with the empty buffer, I just do not realize that something does not work as I expected.

I personally would like Node.js to have the same behavior as Web browser, especially in encoding APIs because many times I want to share the encoding logic between Web and server. Throwing TypeError feels confusing for me.

Doesn't it look more like issue with Buffer.from()? new Uint8Array(ab) on detached buffer should throw but Uint8Array.from(ab) should return exactly what's expected in TextDecoder.decode().

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

Error: Found 1 unexpected passes. Consider updating test/wpt/status/encoding.json for these files: streams/decode-utf8.any.js @cola119 could you take a look at this please?

@aduh95 I updated the status accordingly. 89329e62540060b56adeacd6502d1366b9a5d50b PTAL.
(encoding.json was updated in #44101 and at that time streams/decode-utf8.any.js was expected to fail. This PR fixes that issue and I needed to update the status.)

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

Rebased and squashed

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

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

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

@aduh95 PTAL 2007eb5

Landed in 86088ab

PR-URL: #44020 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs-github-bot added encoding Issues and PRs related to the TextEncoder and TextDecoder APIs. needs-ci PRs that need a full CI run. labels Jul 28, 2022

cola119 force-pushed the fix-type-error-of-detached-buffer branch from bbb15af to ad6a134 Compare July 28, 2022 09:08

BridgeAR reviewed Jul 28, 2022

View reviewed changes

cola119 force-pushed the fix-type-error-of-detached-buffer branch from ad6a134 to 6639bec Compare July 30, 2022 04:08

jasnell approved these changes Aug 28, 2022

View reviewed changes

aduh95 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 28, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2022

github-actions Bot mentioned this pull request Oct 29, 2022

CI Reliability 2022-10-29 nodejs/reliability#414

Open

15 tasks

aduh95 removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 29, 2022

github-actions Bot mentioned this pull request Oct 30, 2022

CI Reliability 2022-10-30 nodejs/reliability#415

Open

15 tasks

cola119 force-pushed the fix-type-error-of-detached-buffer branch from 6639bec to 89329e6 Compare October 30, 2022 04:43

aduh95 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 30, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 30, 2022

cola119 force-pushed the fix-type-error-of-detached-buffer branch 2 times, most recently from 684f18a to a154022 Compare October 30, 2022 23:38

cola119 added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 30, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 30, 2022

github-actions Bot mentioned this pull request Oct 31, 2022

CI Reliability 2022-10-31 nodejs/reliability#416

Open

19 tasks

cola119 removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 31, 2022

cola119 added 2 commits October 31, 2022 22:27

lib: fix TypeError when converting a detached buffer source

a8ed04f

fixup! lib: fix TypeError when converting a detached buffer source

2007eb5

cola119 force-pushed the fix-type-error-of-detached-buffer branch from a154022 to 2007eb5 Compare October 31, 2022 13:32

cola119 added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2022

github-actions Bot mentioned this pull request Nov 1, 2022

CI Reliability 2022-11-01 nodejs/reliability#417

Open

17 tasks

aduh95 approved these changes Nov 1, 2022

View reviewed changes

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 1, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 1, 2022

nodejs-github-bot merged commit 86088ab into nodejs:main Nov 1, 2022

RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022

lib: fix TypeError when converting a detached buffer source

d172669

PR-URL: #44020 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

RafaelGSS mentioned this pull request Nov 10, 2022

v19.1.0 proposal #45269

Merged

danielleadams mentioned this pull request Dec 30, 2022

v18.13.0 release proposal #46025

Merged

danielleadams pushed a commit that referenced this pull request Dec 30, 2022

lib: fix TypeError when converting a detached buffer source

68690ef

PR-URL: #44020 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

danielleadams pushed a commit that referenced this pull request Jan 3, 2023

lib: fix TypeError when converting a detached buffer source

57897f8

PR-URL: #44020 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

lib: fix TypeError when converting a detached buffer source by cola119 · Pull Request #44020 · nodejs/node · GitHub

Uh oh!

Uh oh!

lib: fix TypeError when converting a detached buffer source - #44020

lib: fix TypeError when converting a detached buffer source#44020nodejs-github-bot merged 2 commits intonodejs:mainnodejs/node:mainfrom cola119:fix-type-error-of-detached-buffercola119/node:fix-type-error-of-detached-bufferCopy head branch name to clipboard

Conversation

cola119 commented Jul 28, 2022

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

cola119 commented Jul 28, 2022

Uh oh!

LiviaMedeiros commented Jul 28, 2022

Uh oh!

nodejs-github-bot commented Oct 28, 2022

Uh oh!

aduh95 commented Oct 29, 2022

Uh oh!

cola119 commented Oct 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 30, 2022

Uh oh!

cola119 commented Oct 30, 2022

Uh oh!

nodejs-github-bot commented Oct 30, 2022

Uh oh!

nodejs-github-bot commented Oct 31, 2022

Uh oh!

nodejs-github-bot commented Oct 31, 2022

Uh oh!

cola119 commented Nov 1, 2022

Uh oh!

nodejs-github-bot commented Nov 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lib: fix TypeError when converting a detached buffer source#44020
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
cola119:fix-type-error-of-detached-buffer

cola119 commented Oct 30, 2022 •
edited
Loading