Skip to content

stream: simplify Transform stream implementation#32763

Closed
ronag wants to merge 8 commits into
nodejs:masterfrom
nxtedition:transform-simplify
Closed

stream: simplify Transform stream implementation#32763
ronag wants to merge 8 commits into
nodejs:masterfrom
nxtedition:transform-simplify

Conversation

@ronag

@ronag ronag commented Apr 10, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Significantly simplified Transform stream implementation by
using mostly standard stream code.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@ronag ronag force-pushed the transform-simplify branch from aade81a to a934d7f Compare April 10, 2020 15:19
@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. stream Issues and PRs related to the stream subsystem. labels Apr 10, 2020
@ronag ronag force-pushed the transform-simplify branch from a934d7f to ea3509c Compare April 10, 2020 15:23
ronag
ronag commented Apr 10, 2020
View reviewed changes
Comment thread lib/_stream_transform.js Outdated

@ronag ronag Apr 10, 2020

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.

emitting 'error' was a bug

@ronag ronag force-pushed the transform-simplify branch from ea3509c to 88e8a20 Compare April 10, 2020 15:29
@nodejs-github-bot

nodejs-github-bot commented Apr 10, 2020

Copy link
Copy Markdown
Collaborator

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

@ronag ronag added the wip Issues and PRs that are still a work in progress. label Apr 10, 2020
@ronag ronag force-pushed the transform-simplify branch from 88e8a20 to f84621d Compare April 10, 2020 15:51
@ronag
stream: simplify Transform stream implementation
6099382 
Significantly simplified Transform stream implementation by
using mostly standard stream code.
@ronag ronag force-pushed the transform-simplify branch from f84621d to 6099382 Compare April 10, 2020 15:52
@ronag

ronag commented Apr 10, 2020

Copy link
Copy Markdown
Member Author

Some problems in zlib.

@nodejs-github-bot

nodejs-github-bot commented Apr 10, 2020

Copy link
Copy Markdown
Collaborator

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

@ronag ronag removed the wip Issues and PRs that are still a work in progress. label Apr 10, 2020
@nodejs-github-bot

nodejs-github-bot commented Apr 10, 2020

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Apr 10, 2020

Copy link
Copy Markdown
Collaborator

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

@ronag ronag requested review from addaleax, lpinca and mcollina April 10, 2020 17:07
@ronag ronag added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 10, 2020
@ronag

ronag commented Apr 10, 2020

Copy link
Copy Markdown
Member Author

Marking as semver major since minor differences in behavior probably exists.

ronag
ronag commented Apr 10, 2020
View reviewed changes
Comment thread test/parallel/test-zlib-flush-drain.js
ronag
ronag commented Apr 10, 2020
View reviewed changes
Comment thread test/parallel/test-stream2-transform.js
@nodejs-github-bot

nodejs-github-bot commented Apr 10, 2020

Copy link
Copy Markdown
Collaborator

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

@ronag ronag mentioned this pull request Apr 27, 2020
Closed
4 tasks
@jasnell

jasnell commented Apr 28, 2020

Copy link
Copy Markdown
Member

This really should have done a runtime deprecation for _transformState rather than removing it entirely. It's relatively low impact given that, as a semver-major, this won't be pulled in to any release until 15.0.0 but for 14.x we should add a proper deprecation for _transformState.

@ronag ronag mentioned this pull request Apr 28, 2020
Closed
4 tasks
@codebytere codebytere mentioned this pull request Jun 30, 2020
Merged
@BethGriggs BethGriggs mentioned this pull request Sep 1, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca Awaiting requested review from lpinca

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ronag @nodejs-github-bot @mcollina @jasnell @addaleax @targos