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.

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

streams: implement CompressionStream and DecompressionStream #39348

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jul 11, 2021

Builds on the web streams adapters PR #39134 (first commit here is from that PR)

Implements the Compression Streams Web API using the adapters

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jul 11, 2021
lib/internal/webstreams/adapters.js Outdated Show resolved Hide resolved
lib/internal/webstreams/compression.js Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

@devsnek
Copy link
Member

devsnek commented Jul 13, 2021

should this be a separate warning due to wicg draft status?

@jasnell
Copy link
Member Author

jasnell commented Jul 13, 2021

I don't think a separate warning is necessary or really all that helpful. At the most, it won't move out of experimental at the same time as the rest of the webstreams stuff.

@nodejs-github-bot

This comment has been minimized.

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 14, 2021
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.

I think those should exposed via zlib and not via streams/web

lib/internal/webstreams/compression.js Outdated Show resolved Hide resolved
@jasnell
Copy link
Member Author

jasnell commented Jul 14, 2021

@mcollina:

I think those should exposed via zlib and not via streams/web

I'd prefer to keep all of the stream/web related things together. The intent is for these to eventually be lazy-loaded globals anyway.

@nodejs-github-bot

This comment has been minimized.

Signed-off-by: James M Snell <jasnell@gmail.com>
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 14, 2021

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

@jasnell
Copy link
Member Author

jasnell commented Jul 14, 2021

Landed in 09ad64d

@jasnell jasnell closed this Jul 14, 2021
jasnell added a commit that referenced this pull request Jul 14, 2021
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #39348
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@ronag
Copy link
Member

ronag commented Jul 15, 2021

Please don't forget to ping @nodejs/streams

@targos
Copy link
Member

targos commented Jul 17, 2021

Depends on #39134

lpinca added a commit to lpinca/node that referenced this pull request Aug 26, 2021
Documentation for `CompressionStream` and `DecompressionStream` was
erroneously added in nodejs@f57a0e4d8b
and released in version 16.7.0.

Reset the added: version to REPLACEME.

Refs: nodejs#39348
Refs: nodejs#39899
lpinca added a commit to lpinca/node that referenced this pull request Aug 26, 2021
Documentation for `CompressionStream` and `DecompressionStream` was
erroneously added in nodejs@f57a0e4d8b
and released in version 16.7.0.

Reset the `added:` version to `REPLACEME`.

Refs: nodejs#39348
Refs: nodejs#39899
jasnell pushed a commit that referenced this pull request Aug 28, 2021
Documentation for `CompressionStream` and `DecompressionStream` was
erroneously added in f57a0e4d8b
and released in version 16.7.0.

Reset the `added:` version to `REPLACEME`.

Refs: #39348
Refs: #39899

PR-URL: #39901
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants