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

zlib: fix inheritance of DeflateRaw without classes #13370

Closed
wants to merge 1 commit into from

Conversation

mcollina
Copy link
Member

@mcollina mcollina commented Jun 1, 2017

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 mcollina requested a review from jasnell June 1, 2017 13:47
@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Jun 1, 2017
@mcollina mcollina added the zlib Issues and PRs related to the zlib subsystem. label Jun 1, 2017
@mcollina
Copy link
Member Author

mcollina commented Jun 1, 2017

@jasnell
Copy link
Member

jasnell commented Jun 1, 2017

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

return Reflect.construct(type, args, new.target || type);
const res = Reflect.construct(type, args, new.target || type);

if (this) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. Updated.

@mcollina
Copy link
Member Author

mcollina commented Jun 1, 2017

(I stopped the previous CI job)

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

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Seems fine to me

@mcollina
Copy link
Member Author

mcollina commented Jun 1, 2017

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

@mcollina
Copy link
Member Author

mcollina commented Jun 1, 2017

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

@refack
Copy link
Contributor

refack commented Jun 1, 2017

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
Copy link
Member Author

mcollina commented Jun 1, 2017

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

Fixes internal/util createClassWrapper to support inheritance
without using classes. The constructor now needs to be defined
using a Symbol.

Fixes: nodejs#13358
@addaleax
Copy link
Member

addaleax commented Jun 1, 2017

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
Copy link
Member

watilde commented Jun 1, 2017

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

@jasnell
Copy link
Member

jasnell commented Jun 1, 2017

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
Copy link
Member

jasnell commented Jun 1, 2017

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 jasnell mentioned this pull request Jun 1, 2017
2 tasks
@jasnell
Copy link
Member

jasnell commented Jun 1, 2017

I've opened #13374 as an alternative.

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

Fixes: nodejs#13358
Refs: nodejs#13370
@jasnell
Copy link
Member

jasnell commented Jun 5, 2017

Closing in favor of #13374 which just landed.

@jasnell jasnell closed this Jun 5, 2017
jasnell added a commit that referenced this pull request Jun 5, 2017
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
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
util Issues and PRs related to the built-in util module. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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