Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

stream: improve Readable legacy compat #51470

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

stream: improve Readable legacy compat #51470

wants to merge 3 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jan 15, 2024

The popular stream-shift library accesses internal Readable state which has been modified.

Refs: googleapis/nodejs-storage#2391
Refs: mafintosh/stream-shift#10

@ronag ronag requested review from mcollina and aduh95 January 15, 2024 07:38
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jan 15, 2024
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
@ronag
stream: improve Readable legacy compat
3d23d5b 
The popular stream-shift library accesses internal Readable
state which has been modified.

Refs: googleapis/nodejs-storage#2391
Refs: mafintosh/stream-shift#10
PR-URL: nodejs#51470
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
@ronag
stream: improve Readable legacy compat
98db21e 
The popular stream-shift library accesses internal Readable
state which has been modified.

Refs: googleapis/nodejs-storage#2391
Refs: mafintosh/stream-shift#10
PR-URL: nodejs#51470
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
@ronag
stream: improve Readable legacy compat
377e75a 
The popular stream-shift library accesses internal Readable
state which has been modified.

Refs: googleapis/nodejs-storage#2391
Refs: mafintosh/stream-shift#10
PR-URL: nodejs#51470
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
@ronag
stream: improve Readable legacy compat
e833e46 
The popular stream-shift library accesses internal Readable
state which has been modified.

Refs: googleapis/nodejs-storage#2391
Refs: mafintosh/stream-shift#10
PR-URL: nodejs#51470
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
@ronag
stream: improve Readable legacy compat
61c8a06 
The popular stream-shift library accesses internal Readable
state which has been modified.

Refs: googleapis/nodejs-storage#2391
Refs: mafintosh/stream-shift#10
PR-URL: nodejs#51470
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
@ronag
stream: improve Readable legacy compat
e824b6b 
The popular stream-shift library accesses internal Readable
state which has been modified.

Refs: googleapis/nodejs-storage#2391
Refs: mafintosh/stream-shift#10
PR-URL: nodejs#51470
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
@ronag
stream: improve Readable legacy compat
371f70c 
The popular stream-shift library accesses internal Readable
state which has been modified.

Refs: googleapis/nodejs-storage#2391
Refs: mafintosh/stream-shift#10
PR-URL: nodejs#51470
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
@ronag
stream: improve Readable legacy compat
dc94a20 
The popular stream-shift library accesses internal Readable
state which has been modified.

Refs: googleapis/nodejs-storage#2391
Refs: mafintosh/stream-shift#10
PR-URL: nodejs#51470
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
@ronag
stream: improve Readable legacy compat
19c50d8 
The popular stream-shift library accesses internal Readable
state which has been modified.

Refs: googleapis/nodejs-storage#2391
Refs: mafintosh/stream-shift#10
PR-URL: nodejs#51470
ronag added a commit to nxtedition/node that referenced this pull request Jan 15, 2024
@ronag
stream: improve Readable legacy compat
9efb0cd 
The popular stream-shift library accesses internal Readable
state which has been modified.

Refs: googleapis/nodejs-storage#2391
Refs: mafintosh/stream-shift#10
PR-URL: nodejs#51470
@ronag
stream: improve Readable legacy compat
fe8212b 
The popular stream-shift library accesses internal Readable
state which has been modified.

Refs: googleapis/nodejs-storage#2391
Refs: mafintosh/stream-shift#10
PR-URL: nodejs#51470
@ronag
Copy link
Member Author

ronag commented Jan 15, 2024

Would be easiest if @mafintosh fixed in https://github.com/mafintosh/stream-shift?

mcollina
mcollina reviewed Jan 16, 2024
View reviewed changes
lib/internal/streams/readable.js
@@ -1568,6 +1572,49 @@ ObjectDefineProperties(ReadableState.prototype, {
}
},
},

// Legacy compat
Copy link
Member

Choose a reason for hiding this comment

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

I would add a pending deprecation warning and a runtime deprecation in v22.

Copy link
Member

Choose a reason for hiding this comment

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

Agree

mcollina
mcollina approved these changes Jan 16, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 25, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 25, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels May 11, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 11, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@aduh95
Copy link
Contributor

aduh95 commented May 11, 2024

Seemingly related test failure

not ok 3725 parallel/test-zlib-brotli-16GB
  ---
  duration_ms: 754.97000
  severity: fail
  exitcode: 1
  stack: |-
    node:internal/streams/readable:1583
            proxy = new Proxy(r[kBuffer], {
                               ^
    
    TypeError: Cannot read properties of undefined (reading 'Symbol(kBuffer)')
        at ReadableState.get (node:internal/streams/readable:1583:28)
        at Timeout.<anonymous> (/home/iojs/build/workspace/node-test-commit-arm/test/parallel/test-zlib-brotli-16GB.js:22:38)
        at Timeout._onTimeout (/home/iojs/build/workspace/node-test-commit-arm/test/common/index.js:470:15)
        at listOnTimeout (node:internal/timers:573:17)
        at process.processTimers (node:internal/timers:514:7)
    
    Node.js v23.0.0-pre
  ...

@aduh95 aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rluvaton rluvaton rluvaton left review comments

@mcollina mcollina mcollina approved these changes

@benjamingr benjamingr benjamingr approved these changes

@aduh95 aduh95 Awaiting requested review from aduh95

Assignees
No one assigned
Labels
needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ronag @nodejs-github-bot @aduh95 @mcollina @benjamingr @rluvaton @lpinca