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: fix Writable instanceof for subclasses #14945

Closed
wants to merge 1 commit into
base: master
from

Conversation

@addaleax
Member

addaleax commented Aug 19, 2017

The current custom instanceof for Writable subclasses previously
returned false positives for instances of other subclasses of
Writable because it was inherited by these subclasses.

Fixes: #14943

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

stream

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Aug 19, 2017

Member

LGTM, would love it if you added a test for this.

Member

benjamingr commented Aug 19, 2017

LGTM, would love it if you added a test for this.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Aug 19, 2017

Member

@benjamingr Can you be more specific? The test in this PR is basically equivalent to the code snippet from the issue…

Member

addaleax commented Aug 19, 2017

@benjamingr Can you be more specific? The test in this PR is basically equivalent to the code snippet from the issue…

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Aug 19, 2017

Member

Nevermind, I see it is already covered for the reverse case.

Member

benjamingr commented Aug 19, 2017

Nevermind, I see it is already covered for the reverse case.

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Aug 19, 2017

Member

@addaleax made a suggestion for better assertion fail messages that were just out of the scope of this PR, but I felt should be remedied.
Obviously feel free to push this out if you don't like it.

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

Member

refack commented Aug 19, 2017

@addaleax made a suggestion for better assertion fail messages that were just out of the scope of this PR, but I felt should be remedied.
Obviously feel free to push this out if you don't like it.

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

@refack

refack approved these changes Aug 19, 2017

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Aug 19, 2017

Member

Yea, removed it. If you want, open a different PR, it’s barely going to conflict.

Member

addaleax commented Aug 19, 2017

Yea, removed it. If you want, open a different PR, it’s barely going to conflict.

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Aug 19, 2017

Member

Yea, removed it. If you want, open a different PR, it’s barely going to conflict.

Ack

Member

refack commented Aug 19, 2017

Yea, removed it. If you want, open a different PR, it’s barely going to conflict.

Ack

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 23, 2017

Member

There's too much red in that previous CI run. Trying again: https://ci.nodejs.org/job/node-test-pull-request/9799/ https://ci.nodejs.org/job/node-test-pull-request/9800/

Also, this needs a rebase

Member

jasnell commented Aug 23, 2017

There's too much red in that previous CI run. Trying again: https://ci.nodejs.org/job/node-test-pull-request/9799/ https://ci.nodejs.org/job/node-test-pull-request/9800/

Also, this needs a rebase

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 23, 2017

Member

Added a commit to resolve the merge conflict. Feel free to drop the extra commit if you'd like @addaleax

Member

jasnell commented Aug 23, 2017

Added a commit to resolve the merge conflict. Feel free to drop the extra commit if you'd like @addaleax

stream: fix Writable instanceof for subclasses
The current custom instanceof for `Writable` subclasses previously
returned false positives for instances of *other* subclasses of
`Writable` because it was inherited by these subclasses.

Fixes: #14943
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Aug 23, 2017

Member

I don’t think CI handles merge commits well?

Anyway, I pushed a rebased version

Member

addaleax commented Aug 23, 2017

I don’t think CI handles merge commits well?

Anyway, I pushed a rebased version

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax
Member

addaleax commented Aug 23, 2017

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Aug 24, 2017

Member

Landed in 7ce2555

Member

addaleax commented Aug 24, 2017

Landed in 7ce2555

@addaleax addaleax closed this Aug 24, 2017

@addaleax addaleax deleted the addaleax:writable-x branch Aug 24, 2017

addaleax added a commit that referenced this pull request Aug 24, 2017

stream: fix Writable instanceof for subclasses
The current custom instanceof for `Writable` subclasses previously
returned false positives for instances of *other* subclasses of
`Writable` because it was inherited by these subclasses.

Fixes: #14943
PR-URL: #14945
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax added a commit to addaleax/ayo that referenced this pull request Aug 25, 2017

stream: fix Writable instanceof for subclasses
The current custom instanceof for `Writable` subclasses previously
returned false positives for instances of *other* subclasses of
`Writable` because it was inherited by these subclasses.

Fixes: nodejs/node#14943
PR-URL: nodejs/node#14945
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax added a commit to ayojs/ayo that referenced this pull request Aug 28, 2017

stream: fix Writable instanceof for subclasses
The current custom instanceof for `Writable` subclasses previously
returned false positives for instances of *other* subclasses of
`Writable` because it was inherited by these subclasses.

Fixes: nodejs/node#14943
PR-URL: nodejs/node#14945
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added a commit that referenced this pull request Sep 10, 2017

stream: fix Writable instanceof for subclasses
The current custom instanceof for `Writable` subclasses previously
returned false positives for instances of *other* subclasses of
`Writable` because it was inherited by these subclasses.

Fixes: #14943
PR-URL: #14945
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@MylesBorins MylesBorins referenced this pull request Sep 10, 2017

Merged

v8.5.0 proposal #15308

MylesBorins added a commit that referenced this pull request Sep 12, 2017

stream: fix Writable instanceof for subclasses
The current custom instanceof for `Writable` subclasses previously
returned false positives for instances of *other* subclasses of
`Writable` because it was inherited by these subclasses.

Fixes: #14943
PR-URL: #14945
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 20, 2017

Member

@addaleax should we backport to v6.x? It seems like this could change behavior, even if it were to the correct behavior

Member

MylesBorins commented Sep 20, 2017

@addaleax should we backport to v6.x? It seems like this could change behavior, even if it were to the correct behavior

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Sep 20, 2017

Member

@MylesBorins Yes, I am confident that we should backport this fix.

Member

addaleax commented Sep 20, 2017

@MylesBorins Yes, I am confident that we should backport this fix.

MylesBorins added a commit that referenced this pull request Sep 20, 2017

stream: fix Writable instanceof for subclasses
The current custom instanceof for `Writable` subclasses previously
returned false positives for instances of *other* subclasses of
`Writable` because it was inherited by these subclasses.

Fixes: #14943
PR-URL: #14945
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 20, 2017

Member

done :D 🎉

Member

MylesBorins commented Sep 20, 2017

done :D 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment