Skip to content

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..

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
Contributor

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