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

zlib: fix inheritance of DeflateRaw without classes by mcollina · Pull Request #13370 · nodejs/node · GitHub

/ node Public

zlib: fix inheritance of DeflateRaw without classes - #13370

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

zlib: fix inheritance of DeflateRaw without classes#13370
mcollina wants to merge 1 commit into
nodejs:masterfrom
mcollina:fix-zlib

Conversation

mcollina commented Jun 1, 2017

Copy link
Copy Markdown
Member

Fixes internal/util createClassWrapper to support inheritance
without using classes.

Fixes: #13358

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)

zlib

mcollina requested a review from jasnell June 1, 2017 13:47
nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Jun 1, 2017
mcollina mentioned this pull request Jun 1, 2017
Closed
mcollina added the zlib Issues and PRs related to the zlib subsystem. label Jun 1, 2017

mcollina commented Jun 1, 2017

Copy link
Copy Markdown
Member Author

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

jasnell commented Jun 1, 2017

Copy link
Copy Markdown
Member

Change looks good, testing it really quick locally before signing off..

addaleax reviewed Jun 1, 2017
View reviewed changes
Comment thread lib/internal/util.js Outdated

addaleax Jun 1, 2017

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

if (this && !new.target)? Otherwise this would also set the prototype for cases like new Class(), right?

mcollina Jun 1, 2017

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

yes. Updated.

mcollina commented Jun 1, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

(I stopped the previous CI job)

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

addaleax approved these changes Jun 1, 2017
View reviewed changes

addaleax left a comment

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

Seems fine to me

mcollina commented Jun 1, 2017

Copy link
Copy Markdown
Member Author

do you think the commit message is ok? or it should be tagged as util?

watilde mentioned this pull request Jun 1, 2017
Closed

mcollina commented Jun 1, 2017

Copy link
Copy Markdown
Member Author

there was a bug in my test, and it is not working as expected, i.e. it is not fixing the bug :(.

refack commented Jun 1, 2017

Copy link
Copy Markdown
Contributor

I'm running a CITGM on this, it seems a little "breaky" (explicitly calling Reflect.setPrototypeOf(this, res);)
https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/851/

mcollina commented Jun 1, 2017

Copy link
Copy Markdown
Member Author

Updated. This is more controversial, so @addaleax @watilde you might want to recheck this.

zlib: fix inheritance of DeflateRaw without class
fbfe686 
Fixes internal/util createClassWrapper to support inheritance
without using classes. The constructor now needs to be defined
using a Symbol.

Fixes: nodejs#13358

addaleax commented Jun 1, 2017

Copy link
Copy Markdown
Member

Eh, yes, this is a bit icky. The code still LGTM but I think I might prefer just going back to using functions instead of classes instead…

watilde commented Jun 1, 2017

Copy link
Copy Markdown
Member

I just checked it quickly and it works in node-crc32-stream as well, but I thought the same thing with @addaleax.

jasnell commented Jun 1, 2017

Copy link
Copy Markdown
Member

The fix is a bit unfortunate but it definitely makes sense in that this approach does essentially the same thing as the old function approach. @mcollina is away at the moment but I'm going through and seeing if there's a way of making the fix cleaner

jasnell commented Jun 1, 2017

Copy link
Copy Markdown
Member

In general... I think reverting back to using Functions is likely going to be the best bet long term... it is unfortunate, and is not going to be a clean revert... I'll work on that today

jasnell mentioned this pull request Jun 1, 2017
Closed
2 tasks

jasnell commented Jun 1, 2017

Copy link
Copy Markdown
Member

I've opened #13374 as an alternative.

jasnell added a commit to jasnell/node that referenced this pull request Jun 1, 2017
zlib: revert back to Functions
72571d2 
Using ES6 Classes broke userland code. Revert back to functions.

Fixes: nodejs#13358
Refs: nodejs#13370
dehimer mentioned this pull request Jun 2, 2017
Closed
dehimer mentioned this pull request Jun 2, 2017
Closed

jasnell commented Jun 5, 2017

Copy link
Copy Markdown
Member

Closing in favor of #13374 which just landed.

jasnell closed this Jun 5, 2017
jasnell added a commit that referenced this pull request Jun 5, 2017
zlib: revert back to Functions
7024c5a 
Using ES6 Classes broke userland code. Revert back to functions.

PR-URL: #13374
Fixes: #13358
Ref: #13370
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
jasnell added a commit that referenced this pull request Jun 5, 2017
zlib: revert back to Functions
6aeb555 
Using ES6 Classes broke userland code. Revert back to functions.

PR-URL: #13374
Fixes: #13358
Ref: #13370
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
liyatang mentioned this pull request Jun 26, 2017
Open
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
watilde watilde approved these changes
jasnell Awaiting requested review from jasnell

Assignees

No one assigned

Labels

util Issues and PRs related to the built-in util module. zlib Issues and PRs related to the zlib subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

v8.0.0 — zlib.DeflateRaw only extensible via class keyword

6 participants

Back | FazBrowse Home | New Git URL