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

stream: extract Readable.from in its own file #30140

Closed
wants to merge 1 commit into from

Conversation

@mcollina
Copy link
Member

mcollina commented Oct 26, 2019

In readable-stream, we cannot release the new version built from Node 10.17.0 because IE11 does not support generators needed for async-await to be transpiled correctly. readable-stream is all about backward compatibility, so it makes sense to still support IE11, even if we’ll likely remove support in the next major (built from Node 12).

This PR moves Readable.from into its own file, so we can replace it via a browser tag in package.json, and not ship from in the browser bundle for now.

See: nodejs/readable-stream#420

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
@mcollina

This comment has been minimized.

Copy link
Member Author

mcollina commented Oct 26, 2019

@nodejs-github-bot

This comment has been minimized.

@@ -1209,40 +1210,8 @@ function endReadableNT(state, stream) {
}

Readable.from = function(iterable, opts) {

This comment has been minimized.

Copy link
@devsnek

devsnek Oct 27, 2019

Member

could we just do Readable.from = require('internal/streams/from') now that we have core snapshots?

This comment has been minimized.

Copy link
@mcollina

mcollina Oct 27, 2019

Author Member

I need to backport this to 10, I don't think we have those there yet.
We can remove this type of "postponed loading" everywhere I think - would you mind to open an issue? We'd need to verify it does not slow thing down.

mcollina added a commit that referenced this pull request Oct 29, 2019
See: nodejs/readable-stream#420

PR-URL: #30140
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
@mcollina

This comment has been minimized.

Copy link
Member Author

mcollina commented Oct 29, 2019

Landed in 0099529

@mcollina mcollina closed this Oct 29, 2019
@mcollina mcollina deleted the mcollina:extract-from branch Oct 29, 2019
targos added a commit that referenced this pull request Nov 5, 2019
See: nodejs/readable-stream#420

PR-URL: #30140
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
@targos targos mentioned this pull request Nov 5, 2019
targos added a commit that referenced this pull request Nov 8, 2019
See: nodejs/readable-stream#420

PR-URL: #30140
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
targos added a commit that referenced this pull request Nov 10, 2019
See: nodejs/readable-stream#420

PR-URL: #30140
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
targos added a commit that referenced this pull request Nov 10, 2019
See: nodejs/readable-stream#420

PR-URL: #30140
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
targos added a commit that referenced this pull request Nov 11, 2019
See: nodejs/readable-stream#420

PR-URL: #30140
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
@mcollina

This comment has been minimized.

Copy link
Member Author

mcollina commented Nov 18, 2019

@BethGriggs when are you assembling the next Node 10 release? I'd need to have this included.

BethGriggs added a commit that referenced this pull request Dec 4, 2019
See: nodejs/readable-stream#420

PR-URL: #30140
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
@BethGriggs

This comment has been minimized.

Copy link
Member

BethGriggs commented Dec 4, 2019

@mcollina, I've just landed this on v10.x-staging - I hope to have the Release proposal open today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.