GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Migrate _stream_readable errors to internal/errors by benhalverson · Pull Request #15042 · nodejs/node

Conversation

@benhalverson

@benhalverson benhalverson commented Aug 26, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Ref #11273

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows [commit guidelines]
Affected core subsystem(s)

_stream_readable

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originating in Node.js core. stream Issues and PRs related to Node.js streams. labels Aug 26, 2017
@benhalverson

benhalverson commented Aug 26, 2017

Copy link
Copy Markdown
Member Author

@jasnell I wasn't able to get the tests to pass...

Path: parallel/test-stream-unshift-read-race
ok
assert.js:621
      throw actual;
      ^

Error [ERR_STREAM_PUSH_AFTER_EOF]: stream.push() after EOF
    at readableAddChunk (_stream_readable.js:244:30)
    at Readable.push (_stream_readable.js:212:10)
    at /Users/benhalverson/projects/node/test/parallel/test-stream-unshift-read-race.js:72:7
    at tryBlock (assert.js:591:5)
    at innerThrows (assert.js:610:18)
    at Function.throws (assert.js:637:3)
    at pushError (/Users/benhalverson/projects/node/test/parallel/test-stream-unshift-read-race.js:71:10)
    at Timeout._onTimeout (/Users/benhalverson/projects/node/test/parallel/test-stream-unshift-read-race.js:64:25)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
Command: out/Release/node /Users/benhalverson/projects/node/test/parallel/test-stream-unshift-read-race.js
[02:20|% 100|+ 1799|-   1]: Done
make: *** [test] Error 1

my attempt...

function pushError() {
  assert.throws(function() {
    r.push(Buffer.allocUnsafe(1));
  }, /^ERR_STREAM_PUSH_AFTER_EOF$/);
}


const w = stream.Writable();
const written = [];
w._write = function(chunk, encoding, cb) {
  written.push(chunk.toString());
  cb();
};

r.on('end', common.mustCall(function() {
  assert.throws(function() {
    r.unshift(Buffer.allocUnsafe(1));
  }, /^ERR_STREAM_UNSHIFT_AFTER_END_EVENT$/);
  w.end();
}));
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell

jasnell commented Aug 26, 2017

Copy link
Copy Markdown
Member

Ping @nodejs/streams @mcollina

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina

mcollina commented Aug 26, 2017

Copy link
Copy Markdown
Member

As stated elsewhere, I'm very 👎 on this change until we have a clear strategy on how to keep the same behavior on readable-stream (it needs to support old browsers were inheriting from Error does not work as expected).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@benhalverson

benhalverson commented Aug 26, 2017

Copy link
Copy Markdown
Member Author

@mcollina thanks for the feedback.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@refack refack added the blocked PRs that are blocked by other issues or PRs. label Aug 26, 2017
@refack

refack commented Aug 26, 2017

Copy link
Copy Markdown
Contributor

Hello @benhalverson and welcome. Thank you for the contribution 🥇
AFAICT on the face of it this PR is good, it's just a matter of timing and coordination with the readable-stream package.

I've marked it as blocked until a compatibility strategy is finalized.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell
jasnell reviewed Aug 29, 2017
View reviewed changes
Comment thread lib/internal/errors.js Outdated

@jasnell jasnell Aug 29, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can you please put this in alphabetical order :-) thank you!
Also, I believe these need line wrapped at 80 chars. Run make lint to check

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
jasnell
jasnell reviewed Aug 29, 2017
View reviewed changes
Comment thread lib/_stream_readable.js Outdated

@jasnell jasnell Aug 29, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

s/ERR_UNDERSCORE_READ_NOT_IMPLEMENTED/ERR_STREAM_READ_NOT_IMPLEMENTED

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
jasnell
jasnell reviewed Aug 29, 2017
View reviewed changes
Comment thread lib/_stream_readable.js Outdated

@jasnell jasnell Aug 29, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

s/ERR_END_READABLE_CALLED_ON_NONEMPTY_STREAM/ERR_STREAM_READABLE_CALLED_ON_NONEMPTY

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@jasnell

jasnell commented Aug 29, 2017

Copy link
Copy Markdown
Member

To explain @refack's comments a bit more. The streams module in core is kept in sync with the standalone readable-streams module. Because of that, any changes to the streams code must be kept in sync with that module. Because the internal/errors bit is very specific to Node.js core, we have not started migrating the streams code over to use it.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina mcollina mentioned this pull request Sep 12, 2017
Stream WG meeting - September 2017 nodejs/readable-stream#309
Closed
@mcollina mcollina removed the blocked PRs that are blocked by other issues or PRs. label Sep 12, 2017
@mcollina

mcollina commented Sep 12, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

At the latest streams wg we decided to unblock this. We would like this (and the equals for Writable, Duplex and Transform) to ship in Node 9.

Ref: nodejs/readable-stream#309 (comment)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina

mcollina commented Sep 12, 2017

Copy link
Copy Markdown
Member

@benhalverson I think you can use common.expectsError to check for the error code: https://github.com/nodejs/node/blob/master/test/common/index.js#L696.

See https://github.com/nodejs/node/blob/c2e838ee13244dd3dbd98ed86ef1966fe7cd90f1/test/parallel/test-require-invalid-package.js as an example.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@benhalverson

benhalverson commented Sep 12, 2017

Copy link
Copy Markdown
Member Author

Thanks @mcollina I'll take a look at the example

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina

mcollina commented Sep 18, 2017

Copy link
Copy Markdown
Member

@benhalverson what is the status of this? Would you mind updating to common.expectsError({ code: 'CODE' })?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mcollina
mcollina reviewed Sep 18, 2017
View reviewed changes
Comment thread lib/internal/errors.js Outdated

@mcollina mcollina Sep 18, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

why is this commented out?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@benhalverson benhalverson Sep 18, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I was testing and forgot to uncomment

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@benhalverson benhalverson Sep 20, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I could use some assistance on this test...
current test

function pushError() {
  assert.throws(function() {
    r.push(Buffer.allocUnsafe(1));
  }, /^ERR_STREAM_PUSH_AFTER_EOF/);
}

What I have tried so far:

function pushError() {
   common.expectsError({
     code: 'ERR_STREAM_PUSH_AFTER_EOF',
     message: 'stream.push() after EOF'
    });
  }

This gives the following error.

Path: parallel/test-stream-unshift-read-race
ok
events.js:182
      throw er; // Unhandled 'error' event
      ^

Error: stream.push() after EOF
    at readableAddChunk (_stream_readable.js:243:30)
    at Readable.push (_stream_readable.js:211:10)
    at pushError (/Users/benhalverson/projects/node/test/parallel/test-stream-unshift-read-race.js:77:9)
    at Timeout._onTimeout (/Users/benhalverson/projects/node/test/parallel/test-stream-unshift-read-race.js:64:25)
    at ontimeout (timers.js:471:11)
    at tryOnTimeout (timers.js:305:5)
    at Timer.listOnTimeout (timers.js:265:5)

Without r.push(Buffer.allocUnsafe(1)); I get this error.

Path: parallel/test-stream-unshift-read-race
Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
    at Object.exports.mustCall (/Users/benhalverson/projects/node/test/common/index.js:482:10)
    at Object.expectsError (/Users/benhalverson/projects/node/test/common/index.js:707:27)
    at pushError (/Users/benhalverson/projects/node/test/parallel/test-stream-unshift-read-race.js:78:14)
    at Timeout._onTimeout (/Users/benhalverson/projects/node/test/parallel/test-stream-unshift-read-race.js:64:25)
    at ontimeout (timers.js:471:11)
    at tryOnTimeout (timers.js:305:5)
    at Timer.listOnTimeout (timers.js:265:5)
0: asdfasdfas
1: 1234dfasdf
2: 1234asdfas
3: 1234dfasdf
4: 1234asdfas
5: 1234dfasdf
6: 1234asdfas
7: 1234dfasdf
8: 1234asdfas
9: 1234dfasdf
a: 1234asdfas
b: 1234dfasdf
c: 1234asdfas
d: 1234dfasdf
e: 1234asdfas
f: 1234dfasdf
g: 1234asdfa
h: 1234

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@benhalverson benhalverson Sep 20, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I figured it out @mcollina. Reading the docs helped 👍 https://github.com/nodejs/node/blob/master/doc/guides/using-internal-errors.md

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@mcollina

mcollina commented Sep 20, 2017

Copy link
Copy Markdown
Member

Something went wrong with your latest git operations. Can you please rebase?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina

mcollina commented Sep 21, 2017

Copy link
Copy Markdown
Member

I would recommend that you take the changes you have done, start with a fresh branch, and do a force push here.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@benhalverson

benhalverson commented Sep 21, 2017

Copy link
Copy Markdown
Member Author

I was thinking the same... I did the rebase got a bunch of conflicts and it ended up like this after I fixed the conflicts ¯_(ツ)_/¯

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@BridgeAR

BridgeAR commented Sep 24, 2017

Copy link
Copy Markdown
Member

@benhalverson against what branch did you rebase? I am not sure how you set up your clone but I guess you should rebase like git rebase -i upstream/master.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@benhalverson

benhalverson commented Sep 25, 2017

Copy link
Copy Markdown
Member Author

@BridgeAR I used my local version of node and rebased against upstream/master.
After I committed my local changes I typed git rebase upstream master git applied all the upstream changes then added mine and also showed a bunch of conflicts on files I didn't touch. I used my IDE (webstorm) to resolve the conflicts by accepting their changes and then pushed to my local branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mcollina
mcollina reviewed Sep 25, 2017
View reviewed changes
Comment thread test/parallel/test-stream-readable-with-unimplemented-_read.js Outdated

@mcollina mcollina Sep 25, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

can you use common.expectsError here as well?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@benhalverson benhalverson Oct 1, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I've updated this test

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@mcollina

mcollina commented Sep 25, 2017

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mcollina
mcollina approved these changes Sep 25, 2017
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM with a nit

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
mcollina
mcollina reviewed Sep 25, 2017
View reviewed changes
Comment thread lib/_stream_readable.js Outdated

@mcollina mcollina Sep 25, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This should be ERR_STREAM_ENDREADABLE_CALLED_ON_NONEMPTY.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@benhalverson benhalverson Oct 1, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I've updated this line

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
mcollina
mcollina requested changes Sep 25, 2017
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

There is the error code to be changed, I'm moving my feedback to "request changes" instead of approved.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 25, 2017
@benhalverson benhalverson mentioned this pull request Oct 1, 2017
streams testing not passing nodejs/help#852
Closed
BridgeAR
BridgeAR reviewed Oct 1, 2017
View reviewed changes

@BridgeAR BridgeAR left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please remove the empty template string.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread test/parallel/test-stream-readable-with-unimplemented-_read.js Outdated

@BridgeAR BridgeAR Oct 1, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Just as a suggestion - it would be nicer to use

common.expectsError(throwingFn, errorObj)
// instead of
assert.throws(throwingFn, common.expectsError(errorObj))

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread lib/internal/errors.js Outdated

@BridgeAR BridgeAR Oct 1, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

A empty template string?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@benhalverson

benhalverson commented Oct 2, 2017

Copy link
Copy Markdown
Member Author

Oops 😂

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@benhalverson

benhalverson commented Oct 4, 2017

Copy link
Copy Markdown
Member Author

@mcollina is there anything else I need to change?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mcollina
mcollina approved these changes Oct 6, 2017
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@mcollina
mcollina requested a review from a team October 6, 2017 21:08
addaleax
addaleax approved these changes Oct 7, 2017
View reviewed changes
@addaleax

addaleax commented Oct 7, 2017

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-commit/12849/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mhdawson
mhdawson approved these changes Oct 11, 2017
View reviewed changes

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@mhdawson

mhdawson commented Oct 11, 2017

Copy link
Copy Markdown
Member

Looks like the CI job had conflicts even though the PR says it can be merge automatically.

@benhalverson could you squash the commits down 1 one (I'm thinking maybe something that is added/them removed is the source of the conflicts) and then we can run the CI again.

benhalverson reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@benhalverson @joyeecheung
stream: migrate _stream_readable use error codes
b635d1a
@joyeecheung

joyeecheung commented Oct 28, 2017

Copy link
Copy Markdown
Member

I have went ahead and squashed the commits in this PR. CI: https://ci.nodejs.org/job/node-test-pull-request/11041/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@joyeecheung

joyeecheung commented Oct 28, 2017

Copy link
Copy Markdown
Member

Oh, this feature is nice:

https://ci.nodejs.org/job/node-test-linter/13033/console

not ok 11 - /usr/home/iojs/build/workspace/node-test-linter/lib/internal/errors.js
  ---
  message: '"ERR_INVALID_URI" is not documented in doc/api/errors.md'
  severity: error
  data:
    line: 291
    column: 1
    ruleId: documented-errors
  messages:
    - message: doc/api/errors.md does not have an anchor for "ERR_INVALID_URI"
      severity: error
      data:
        line: 291
        column: 1
        ruleId: documented-errors
  ...

@benhalverson Do you have time to document this error code? If not I can do that, just don't want to delay this PR for too long.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gireeshpunathil
gireeshpunathil approved these changes Oct 28, 2017
View reviewed changes
@mcollina

mcollina commented Oct 28, 2017

Copy link
Copy Markdown
Member

@joyeecheung that error code is not present in readable. I think this should land asap before 9.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@joyeecheung

joyeecheung commented Oct 28, 2017

Copy link
Copy Markdown
Member

@mcollina Oh, I think I posted in the wrong thread, this error is from the querystring error migration PR. The errors for this one is

not ok 11 - /usr/home/iojs/build/workspace/node-test-linter/lib/internal/errors.js
  ---
  message: >-
    "ERR_STREAM_ENDREADABLE_CALLED_ON_NONEMPTY" is not documented in
    doc/api/errors.md
  severity: error
  data:
    line: 326
    column: 1
    ruleId: documented-errors
  messages:
    - message: >-
        doc/api/errors.md does not have an anchor for
        "ERR_STREAM_ENDREADABLE_CALLED_ON_NONEMPTY"
      severity: error
      data:
        line: 326
        column: 1
        ruleId: documented-errors
    - message: '"ERR_STREAM_PUSH_AFTER_EOF" is not documented in doc/api/errors.md'
      severity: error
      data:
        line: 328
        column: 1
        ruleId: documented-errors
    - message: doc/api/errors.md does not have an anchor for "ERR_STREAM_PUSH_AFTER_EOF"
      severity: error
      data:
        line: 328
        column: 1
        ruleId: documented-errors
    - message: '"ERR_STREAM_READ_NOT_IMPLEMENTED" is not documented in doc/api/errors.md'
      severity: error
      data:
        line: 329
        column: 1
        ruleId: documented-errors
    - message: >-
        doc/api/errors.md does not have an anchor for
        "ERR_STREAM_READ_NOT_IMPLEMENTED"
      severity: error
      data:
        line: 329
        column: 1
        ruleId: documented-errors
    - message: >-
        "ERR_STREAM_UNSHIFT_AFTER_END_EVENT" is not documented in
        doc/api/errors.md
      severity: error
      data:
        line: 330
        column: 1
        ruleId: documented-errors
    - message: >-
        doc/api/errors.md does not have an anchor for
        "ERR_STREAM_UNSHIFT_AFTER_END_EVENT"
      severity: error
      data:
        line: 330
        column: 1
        ruleId: documented-errors
  ...

Also there are some incorrect use of common.expectsError, I will fix those as well.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

joyeecheung added 3 commits October 28, 2017 18:56
@joyeecheung
fix common.expectsError usage in stream tests
eafcac4
@joyeecheung
delete removed error
cb38372
@joyeecheung
add documentations for stream errors
0ccc7e4
@joyeecheung

joyeecheung commented Oct 28, 2017

Copy link
Copy Markdown
Member

I have fixed the errors and added documentation. New CI: https://ci.nodejs.org/job/node-test-pull-request/11044/

@jasnell @BridgeAR @mcollina @addaleax @gireeshpunathil @mhdawson PTAL

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

refack
refack approved these changes Oct 28, 2017
View reviewed changes

@refack refack left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

FWIW

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
gireeshpunathil
gireeshpunathil approved these changes Oct 28, 2017
View reviewed changes
@Trott

Trott commented Oct 28, 2017

Copy link
Copy Markdown
Member

(CI failures are unrelated build issues.)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina

mcollina commented Oct 29, 2017

Copy link
Copy Markdown
Member

Landed as 88fb359

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mcollina mcollina closed this Oct 29, 2017
mcollina pushed a commit that referenced this pull request Oct 29, 2017
@benhalverson @mcollina
stream: migrate _stream_readable use error codes
88fb359
PR-URL: #15042
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@jasnell jasnell mentioned this pull request Oct 30, 2017
Proposal: 2017-10-31, Version 9.0.0 (Current) #15135
Merged
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@benhalverson @Qard
stream: migrate _stream_readable use error codes
c046552
PR-URL: nodejs/node#15042
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@benhalverson @addaleax
stream: migrate _stream_readable use error codes
6dee9c1
PR-URL: nodejs/node#15042
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@benhalverson
benhalverson deleted the stream_readable_error branch December 15, 2017 04:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell left review comments
@BridgeAR BridgeAR BridgeAR left review comments
@mcollina mcollina mcollina approved these changes
@addaleax addaleax addaleax approved these changes
@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes
+2 more reviewers
@refack refack refack approved these changes
@mhdawson mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

errors Issues and PRs related to JavaScript errors originating in Node.js core. semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to Node.js streams.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

11 participants

@benhalverson @jasnell @mcollina @refack @BridgeAR @addaleax @mhdawson @joyeecheung @Trott @gireeshpunathil @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.