FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

stream: extract Readable.from in its own file by mcollina · Pull Request #30140 · nodejs/node · GitHub

/ node Public

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

Closed
mcollina wants to merge 1 commit into
nodejs:masterfrom
mcollina:extract-from
ConversationCommits (1)ChecksFiles changed
Closed

stream: extract Readable.from in its own file#30140
mcollina wants to merge 1 commit into
nodejs:masterfrom
mcollina:extract-from

Conversation

mcollina commented Oct 26, 2019

Copy link
Copy Markdown
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 added lts Issues and PRs related to Long Term Support releases. lts-watch-v10.x labels Oct 26, 2019
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 commented Oct 26, 2019

Copy link
Copy Markdown
Member Author

cc @vweevers

nodejs-github-bot commented Oct 26, 2019

Copy link
Copy Markdown
Collaborator

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

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

Readable.from = function(iterable, opts) {

devsnek Oct 27, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mcollina Oct 27, 2019

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
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 commented Oct 29, 2019

Copy link
Copy Markdown
Member Author

Landed in 0099529

mcollina closed this Oct 29, 2019
mcollina deleted the extract-from branch October 29, 2019 08:39
targos pushed a commit that referenced this pull request Nov 5, 2019
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 mentioned this pull request Nov 5, 2019
Merged
targos pushed a commit that referenced this pull request Nov 8, 2019
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
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
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 mentioned this pull request Nov 10, 2019
Merged
targos pushed a commit that referenced this pull request Nov 11, 2019
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 commented Nov 18, 2019

Copy link
Copy Markdown
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
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 commented Dec 4, 2019

Copy link
Copy Markdown
Member

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

BethGriggs mentioned this pull request Dec 4, 2019
Closed
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
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 mentioned this pull request Jan 7, 2020
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax approved these changes
cjihrig cjihrig approved these changes
BethGriggs BethGriggs approved these changes
trivikr trivikr approved these changes
mafintosh Awaiting requested review from mafintosh
jasnell Awaiting requested review from jasnell
+1 more reviewer
devsnek devsnek approved these changes
Reviewers whose approvals may not affect merge requirements

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.

7 participants

Back | FazBrowse Home | New Git URL