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.

Sign up for GitHub

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: fix Writable subclass instanceof checks #9088

Closed
wants to merge 3 commits into from
Closed

stream: fix Writable subclass instanceof checks #9088

wants to merge 3 commits into from

Conversation

addaleax
Copy link
Member

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

stream

Description of change

2a4b068 introduced a regression in where checking instanceof would fail for Writable subclasses inside the subclass constructor, i.e. before Writable() was called.

Also, calling null instanceof Writable or undefined instanceof Writable would fail due to accessing the _writableState property of the target object.

This fixes these problems.

Ref: #8834 (comment)

@addaleax
stream: fix Writable subclass instanceof checks
7fe8672 
2a4b068 introduced a regression in where checking
`instanceof` would fail for `Writable` subclasses inside the
subclass constructor, i.e. before `Writable()` was called.

Also, calling `null instanceof Writable` or
`undefined instanceof Writable` would fail due to accessing the
`_writableState` property of the target object.

This fixes these problems.

Ref: nodejs#8834 (comment)
@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Oct 13, 2016
@addaleax addaleax mentioned this pull request Oct 13, 2016
Closed
4 tasks
@Fishrock123 Fishrock123 mentioned this pull request Oct 13, 2016
Closed
@addaleax
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-commit/5601/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/413/

/cc @nodejs/streams

imyller
imyller approved these changes Oct 13, 2016
View reviewed changes
Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

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

LGTM

@MylesBorins
Copy link
Contributor

/cc @mcollina

mcollina
mcollina reviewed Oct 14, 2016
View reviewed changes
lib/_stream_writable.js
@@ -141,9 +141,12 @@ if (typeof Symbol === 'function' && Symbol.hasInstance) {
realHasInstance = Function.prototype[Symbol.hasInstance];
Object.defineProperty(Writable, Symbol.hasInstance, {
value: function(object) {
if (realHasInstance.call(this, object))
return true;

Copy link
Member

Choose a reason for hiding this comment

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

how does this affect LazyTransform?

Copy link
Member Author

Choose a reason for hiding this comment

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

@mcollina It works fine (otherwise there are failing tests). LazyTransform is only affected by the conditional in the Writable constructor, and that’s left unchanged here,

Copy link
Member

Choose a reason for hiding this comment

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

can you update the comment below and maybe move it to the constructor then.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mcollina done, ptal

Fishrock123
Fishrock123 approved these changes Oct 14, 2016
View reviewed changes
Copy link
Contributor

@Fishrock123 Fishrock123 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lgtm if tests are passing, it's better than what is currently public for sure

jasnell
jasnell approved these changes Oct 14, 2016
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM

@evanlucas
Copy link
Contributor

Is this good to land?

@mcollina
Copy link
Member

LGTM

1 similar comment
@evanlucas
Copy link
Contributor

LGTM

@evanlucas
Copy link
Contributor

Landing before the 48h minimum so we can get a non-broken version of v6 out

@evanlucas
Copy link
Contributor

Landed in 7922830

@evanlucas evanlucas closed this Oct 14, 2016
evanlucas pushed a commit that referenced this pull request Oct 14, 2016
@addaleax @evanlucas
stream: fix Writable subclass instanceof checks
7922830 
2a4b068 introduced a regression in where checking
`instanceof` would fail for `Writable` subclasses inside the
subclass constructor, i.e. before `Writable()` was called.

Also, calling `null instanceof Writable` or
`undefined instanceof Writable` would fail due to accessing the
`_writableState` property of the target object.

This fixes these problems.

PR-URL: #9088
Ref: #8834 (comment)
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
evanlucas pushed a commit that referenced this pull request Oct 14, 2016
@addaleax @evanlucas
stream: fix Writable subclass instanceof checks
8b90814 
2a4b068 introduced a regression in where checking
`instanceof` would fail for `Writable` subclasses inside the
subclass constructor, i.e. before `Writable()` was called.

Also, calling `null instanceof Writable` or
`undefined instanceof Writable` would fail due to accessing the
`_writableState` property of the target object.

This fixes these problems.

PR-URL: #9088
Ref: #8834 (comment)
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
evanlucas pushed a commit that referenced this pull request Oct 14, 2016
@addaleax @evanlucas
stream: fix Writable subclass instanceof checks
8c4fab0 
2a4b068 introduced a regression in where checking
`instanceof` would fail for `Writable` subclasses inside the
subclass constructor, i.e. before `Writable()` was called.

Also, calling `null instanceof Writable` or
`undefined instanceof Writable` would fail due to accessing the
`_writableState` property of the target object.

This fixes these problems.

PR-URL: #9088
Ref: #8834 (comment)
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
@evanlucas evanlucas mentioned this pull request Oct 14, 2016
Merged
evanlucas added a commit that referenced this pull request Oct 14, 2016
@evanlucas
2016-10-14, Version 6.8.1 (Current)
c6a397b 
* build: Fix building with shared zlib. (Bradley T. Hughes) [#9077](#9077)
* stream: fix `Writable` subclass instanceof checks (Anna Henningsen) [#9088](#9088)
* timers: fix regression with clearImmediate() (Brian White) [#9086](#9086)

PR-URL: #9104
evanlucas added a commit that referenced this pull request Oct 15, 2016
@evanlucas
2016-10-14, Version 6.8.1 (Current)
113c697 
* build: Fix building with shared zlib. (Bradley T. Hughes) [#9077](#9077)
* stream: fix `Writable` subclass instanceof checks (Anna Henningsen) [#9088](#9088)
* timers: fix regression with clearImmediate() (Brian White) [#9086](#9086)

PR-URL: #9104
@addaleax addaleax deleted the fix-writable-instanceof branch October 15, 2016 23:29
jasnell pushed a commit that referenced this pull request Oct 17, 2016
@addaleax @jasnell
stream: fix Writable subclass instanceof checks
ee9062b 
2a4b068 introduced a regression in where checking
`instanceof` would fail for `Writable` subclasses inside the
subclass constructor, i.e. before `Writable()` was called.

Also, calling `null instanceof Writable` or
`undefined instanceof Writable` would fail due to accessing the
`_writableState` property of the target object.

This fixes these problems.

PR-URL: #9088
Ref: #8834 (comment)
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Oct 17, 2016
@imyller
2016-10-14, Version 6.8.1 (Current)
c84b420 
    * build: Fix building with shared zlib. (Bradley T. Hughes) [#9077](nodejs/node#9077)
    * stream: fix `Writable` subclass instanceof checks (Anna Henningsen) [#9088](nodejs/node#9088)
    * timers: fix regression with clearImmediate() (Brian White) [#9086](nodejs/node#9086)

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Oct 17, 2016
@imyller
2016-10-14, Version 6.8.1 (Current)
aa4dfb9 
    * build: Fix building with shared zlib. (Bradley T. Hughes) [#9077](nodejs/node#9077)
    * stream: fix `Writable` subclass instanceof checks (Anna Henningsen) [#9088](nodejs/node#9088)
    * timers: fix regression with clearImmediate() (Brian White) [#9086](nodejs/node#9086)

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
@zertosh zertosh mentioned this pull request Oct 17, 2016
Closed
@aliscco aliscco mentioned this pull request Nov 29, 2023
Open
@leonardoadame leonardoadame mentioned this pull request Nov 29, 2023
Open
This was referenced Nov 30, 2023
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina left review comments

@imyller imyller imyller approved these changes

@Fishrock123 Fishrock123 Fishrock123 approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
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.
8 participants
@addaleax @MylesBorins @evanlucas @mcollina @jasnell @Fishrock123 @imyller @nodejs-github-bot