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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

mcollina
Copy link
Member

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 mcollina added lts Issues and PRs related to Long Term Support releases. lts-watch-v10.x labels Oct 26, 2019
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. stream Issues and PRs related to the stream subsystem. labels Oct 26, 2019
@mcollina
Copy link
Member Author

cc @vweevers

@nodejs-github-bot
Copy link
Collaborator

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

devsnek
devsnek approved these changes Oct 27, 2019
View reviewed changes
lib/_stream_readable.js
@@ -1209,40 +1210,8 @@ function endReadableNT(state, stream) {
}

Readable.from = function(iterable, opts) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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
@mcollina
stream: extract Readable.from in its own file
0099529 
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
Copy link
Member Author

Landed in 0099529

@mcollina mcollina closed this Oct 29, 2019
@mcollina mcollina deleted the extract-from branch October 29, 2019 08:39
targos pushed a commit that referenced this pull request Nov 5, 2019
@mcollina @targos
stream: extract Readable.from in its own file
87f14e1 
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
Merged
targos pushed a commit that referenced this pull request Nov 8, 2019
@mcollina @targos
stream: extract Readable.from in its own file
81f0fbc 
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 pushed a commit that referenced this pull request Nov 10, 2019
@mcollina @targos
stream: extract Readable.from in its own file
0186847 
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 pushed a commit that referenced this pull request Nov 10, 2019
@mcollina @targos
stream: extract Readable.from in its own file
20f74ec 
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 10, 2019
Merged
targos pushed a commit that referenced this pull request Nov 11, 2019
@mcollina @targos
stream: extract Readable.from in its own file
8a31136 
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
Copy link
Member Author

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

BethGriggs pushed a commit that referenced this pull request Dec 4, 2019
@mcollina @BethGriggs
stream: extract Readable.from in its own file
c4dc53c 
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
Copy link
Member

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

@BethGriggs BethGriggs mentioned this pull request Dec 4, 2019
Closed
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
@mcollina @MylesBorins
stream: extract Readable.from in its own file
894aaa2 
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 BethGriggs mentioned this pull request Jan 7, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@devsnek devsnek devsnek approved these changes

@BethGriggs BethGriggs BethGriggs approved these changes

@trivikr trivikr trivikr approved these changes

@mafintosh mafintosh Awaiting requested review from mafintosh

@jasnell jasnell Awaiting requested review from jasnell

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. lts Issues and PRs related to Long Term Support releases. 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.

None yet
7 participants
@mcollina @nodejs-github-bot @BethGriggs @addaleax @cjihrig @devsnek @trivikr