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

Update to Node v10.17.0 #420

Merged
merged 8 commits into from
Jan 17, 2020
Merged

Update to Node v10.17.0 #420

merged 8 commits into from
Jan 17, 2020

Conversation

mcollina
Copy link
Member

This is important because it adds:

  • Readable.from()
  • Makes async iteration stable

lib/_stream_readable.js Outdated Show resolved Hide resolved
@mcollina
Copy link
Member Author

@vweevers have you got an idea on why the browser tests are not running at all?

@vweevers
Copy link
Contributor

Maybe because it's running on node 13? I haven't tested airtap on 13 yet.

@vweevers
Copy link
Contributor

Looks like the same thing is happening on node 12.

@mcollina Does npm run test-browser-local work locally?

@mcollina
Copy link
Member Author

@vweevers turns out the problem was in the code. They are passing locally now.

@mcollina
Copy link
Member Author

@vweevers there seems to be an issue with IE11. The new Readable.from() internally uses async iteration, which we compile down to generators to support Node 6. However, generators are not available in IE11.

I see a some options:

  1. Drop IE 11, but this would be better done in a semver-major release.
  2. pluck out Readable.from from readable-stream
  3. pluck out Readable.from from the browser build of readable-stream
  4. do a browser build that ship regenerator or analog

I do not see many other options, but maybe there are.

I would really like to ship this update because it will remove the warning for async iteration. It’s about time.

@vweevers
Copy link
Contributor

Drop IE 11, but this would be better done in a semver-major release

+1. For what it's worth, feross's buffer will likely also drop IE11 in a next major, which will then propagate to browserify, airtap, etc.

pluck out Readable.from from readable-stream

-1. Modularity is good, but in this case it's too surprising for users, because readable-stream is meant to mirror node streams. For a browserify build to "just work" it would have to detect Readable.from somehow and shim it separately. Perhaps it's worth a discussion in node. These static methods (from, finished, pipeline, events.once) would be easier to shim and tree-shake if they were separate modules in core.

pluck out Readable.from from the browser build of readable-stream

If you mean disabling it (until the next major) then +1.

do a browser build that ship regenerator or analog

-1. Although regenerator is small, it does add up if multiple modules include it, so I think polyfilling like this should be an application concern.

mcollina added a commit to mcollina/node that referenced this pull request Oct 26, 2019
mcollina added a commit to nodejs/node 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>
targos pushed a commit to nodejs/node 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 pushed a commit to nodejs/node 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 pushed a commit to nodejs/node 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 pushed a commit to nodejs/node 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 pushed a commit to nodejs/node 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>
BethGriggs pushed a commit to nodejs/node 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>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Dec 17, 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
Copy link
Member Author

Updated to Node 10.18.1, let's see how it goes.

@mcollina
Copy link
Member Author

🎉 CI is passing.

@vweevers @mafintosh would you like to take a final look?

@vweevers
Copy link
Contributor

Approved it seconds before your comment :)

I suggest updating the PR title for the record.

@mcollina mcollina merged commit c73f5f6 into master Jan 17, 2020
@mcollina mcollina deleted the node-10-17-0 branch January 17, 2020 16:56
mcollina added a commit that referenced this pull request Jan 17, 2020
* Update to Node v10.17.0

* Browsers might not have Symbol, from edition

* Use Node 12 for airtap

* test passing on browser

* Update to Node 10.18.1

* fixed file patterns for build

* possibly browser fix

* test passing down to Node 6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants