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

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

Closed
iarna opened this issue Jun 1, 2017 · 8 comments
Closed

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

iarna opened this issue Jun 1, 2017 · 8 comments
Labels
zlib Issues and PRs related to the zlib subsystem.

Comments

@iarna
Copy link
Member

iarna commented Jun 1, 2017

  • Version: v8.0.0
  • Platform: Darwin
  • Subsystem: zlib

Extending zlib.DeflateRaw via non-class-keyword doesn't work. You can see this in action with this example:

'use strict'
const zlib = require('zlib');
const inherits = require('util').inherits;

function NotInitialized (options) {
  zlib.DeflateRaw.call(this, options);
  this.prop = true
}
inherits(NotInitialized, zlib.DeflateRaw);
console.log(new NotInitialized())

The above prints out NotInitialized { prop: true }

What I expected it to do, was what this example does:

'use strict'
const zlib = require('zlib');
class Initialized extends zlib.DeflateRaw {
  constructor (options) {
    super(options)
    this.prop = true
  }
}
console.log(new Initialized())

Not doing this initialization means that many stream operations result in crashes. (For example, pipe.)

This shows up in the real world with crc32-stream.

@iarna iarna added the zlib Issues and PRs related to the zlib subsystem. label Jun 1, 2017
@iarna iarna changed the title zlib.deflateRaw not extensible via zlib.deflateRaw only extensible via class keyword Jun 1, 2017
@iarna iarna changed the title zlib.deflateRaw only extensible via class keyword v8.0.0 — zlib.deflateRaw only extensible via class keyword Jun 1, 2017
@iarna iarna changed the title v8.0.0 — zlib.deflateRaw only extensible via class keyword v8.0.0 — zlib.DeflateRaw only extensible via class keyword Jun 1, 2017
@iarna
Copy link
Member Author

iarna commented Jun 1, 2017

This seems to be happening because zlib.DeflateRaw now returns a new object instead of mutating this. While technically valid, it breaks common inheritance patterns. (As seen above.)


<pre><s>
var extend = require('util')._extend
function NotInitialized (options) {
  var stream = zlib.DeflateRaw.call(this, options);
  if (stream) extend(this, stream)
  this.prop = true
}
inherits(NotInitialized, zlib.DeflateRaw);
</s></pre>

Yeah, that doesn't actually work. If `NotInitailized`'s constructor just mutated and returned `stream` then it should be fine, though ofc you don't need the inherits then since you aren't doing any.

@mscdex mscdex added the v8.x label Jun 1, 2017
@mscdex
Copy link
Contributor

mscdex commented Jun 1, 2017

/cc @jasnell

@jasnell
Copy link
Member

jasnell commented Jun 1, 2017

I'm just getting going this morning so I'll have a look a bit more in depth soon, but does this break an existing module or does it just not work as it was expected?

@mcollina
Copy link
Member

mcollina commented Jun 1, 2017

Yes, completely. I have a quick fix ready to go, a PR is coming soon.

@jasnell
Copy link
Member

jasnell commented Jun 1, 2017

Ok. Sigh, we should get those into citgm then.

@mcollina
Copy link
Member

mcollina commented Jun 1, 2017

PR with a fix in #13370.

@iarna I have not verified this with the affected module, I just checked that the example you provided passes.

mcollina added a commit to mcollina/node that referenced this issue Jun 1, 2017
Fixes internal/util createClassWrapper to support inheritance
without using classes. The constructor now needs to be defined
using a Symbol.

Fixes: nodejs#13358
jasnell added a commit to jasnell/node that referenced this issue Jun 1, 2017
Using ES6 Classes broke userland code. Revert back to functions.

Fixes: nodejs#13358
Refs: nodejs#13370
@iarna
Copy link
Member Author

iarna commented Jun 1, 2017

@mcollina I'll give the module I was using a go with the PR in a few hours.

@jasnell
Copy link
Member

jasnell commented Jun 1, 2017

@iarna ... I think #13374 is the direction we're most likely to go on fixing this.

@jasnell jasnell closed this as completed in 7024c5a Jun 5, 2017
jasnell added a commit that referenced this issue 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
zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
4 participants