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

How to inherit from new Buffer implementation #2882

Closed
corentingurtner opened this issue Sep 15, 2015 · 18 comments
Closed

How to inherit from new Buffer implementation #2882

corentingurtner opened this issue Sep 15, 2015 · 18 comments
Assignees
Labels
buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations. question Issues that look for answers.

Comments

@corentingurtner
Copy link

Hi,
I would like to know how a class could inherit from the new Buffer implementation.

I'm trying to upgrade the node-ogg module for node v4 and nan v2.
(Here is my work so far)

I tried this classic inheritance model:

function ogg_packet (buffer) {
  if (!Buffer.isBuffer(buffer)) {
    Buffer.call(this, binding.sizeof_ogg_packet);
  } else {
    Buffer.call(this, buffer);
  }
  if (this.length != binding.sizeof_ogg_packet) {
    throw new Error('"buffer.length" = ' + this.length + ', expected ' + binding.sizeof_ogg_packet);
  }
}
inherits(ogg_packet, Buffer);

But I get this error when trying to access this.length :

Uncaught TypeError: Method Uint8Array.length called on incompatible receiver [object Object]

Thanks

Note: I also asked the question on SO:
http://stackoverflow.com/questions/32555714

@Fishrock123 Fishrock123 added the question Issues that look for answers. label Sep 15, 2015
@ChALkeR ChALkeR added the buffer Issues and PRs related to the buffer subsystem. label Sep 15, 2015
@bnoordhuis
Copy link
Member

ES6 extends should work:

> class C extends Buffer { constructor(size) { super(size) } }
[Function: C]

> (new C(32)).length
32

@corentingurtner
Copy link
Author

Thanks @bnoordhuis ,

I know it can be seen as going backward, but is there a way in ES5 too?

@bnoordhuis
Copy link
Member

I think that's going to be less easy for the same reason that it's difficult to inherit from Array or Uint8Array in ES5. You can maybe hack something together through the __proto__ property.

@trevnorris
Copy link
Contributor

Try something like this:

function Ogg() {
  const ui = new Uint8Array(size);
  Object.setPrototypeOf(ui, Ogg.prototype);
  return ui;
}

Ogg.prototype.__proto__ = Buffer.prototype;
Ogg.__proto__ = Buffer;

Yeah, ugly as sin.

@Fishrock123
Copy link
Contributor

This should probably go into the buffer docs.

@Fishrock123 Fishrock123 added the doc Issues and PRs related to the documentations. label Sep 15, 2015
@corentingurtner
Copy link
Author

Thanks a lot for your answers,

@trevnorris :
I succeed to create an instance this way thanks to your help:

function ogg_packet (buffer) {
  if (!Buffer.isBuffer(buffer)) {
    buffer = new Uint8Array(binding.sizeof_ogg_packet);
  }

  if (buffer.length != binding.sizeof_ogg_packet) {
    throw new Error('"buffer.length" = ' + buffer.length + ', expected ' + binding.sizeof_ogg_packet);
  }

  Object.setPrototypeOf(buffer, ogg_packet.prototype);
  console.log('BUFFER', buffer.toString());
  return buffer;
}
ogg_packet.prototype.__proto__ = Buffer.prototype;
ogg_packet.__proto__ = Buffer;

However, it seems that I can't use Buffer methods (at least toString()) since there is a strict equality check of the instance. For example, trying to print buffer.toString() in the constructor function give me that error:

Uncaught TypeError: argument should be a Buffer
 at TypeError (native)
 at ogg_packet.Buffer.toString (buffer.js:354:23)

@trevnorris
Copy link
Contributor

@corentingurtner That would be from the node::Buffer::HasInstance() check. It's strictly checking the hidden class. Unfortunately there's no way I'm aware of to do proto transversal in the C++ API, similar to instanceof.

/cc @domenic Know of a way to do this proto transversal check in C++?

If there's not a quick fix then let's open a new issue for this.

@domenic
Copy link
Contributor

domenic commented Sep 15, 2015

@trevnorris you can use v8::Object::GetPrototype(). Something like objLocal->GetPrototype()->Get(context, v8_str("constructor")) to get the constructor and then check with Equals against the constructor.

@domenic
Copy link
Contributor

domenic commented Sep 15, 2015

What you really want though is to check that the appropriate internal data is set on the object. Which I think just would be val->IsUint8Array(). Otherwise I can fool it with crap like var notABuffer = {}; notABuffer.__proto__ = Buffer.prototype.

@trevnorris
Copy link
Contributor

@domenic ah yup. sure enough. when I had to reimplement the check I was focused on not allowing users to bypass the instanceof check in JS.

I agree that the check should be simplified down to checking internal data. All methods will work from that point regardless of whether it's actually a Buffer instance. I'll make a PR.

@ben-page
Copy link
Contributor

+1 This stops me from migrating to 4.x.

@trevnorris
Copy link
Contributor

@bnoordhuis using classes won't work:

'use strict';

class Foo extends Buffer {
  constructor(n) { super(n); }

  foo() {
    let cntr = 0;
    for (let i = 0; i < this.length; i++) {
      cntr += this[i];
    }
    return cntr;
  }
}

let foo = new Foo(17).fill('abc');

console.log(foo.foo());  // TypeError: foo.foo is not a function

We are overwriting the prototype in the Buffer constructor, and loosing all the child's class methods. Want to examine this a bit more.

@domenic
Copy link
Contributor

domenic commented Sep 24, 2015

This is fixable once new.target ships in V8. You'd use new.target.prototype instead of Buffer.prototype.

@targos
Copy link
Member

targos commented Sep 24, 2015

We'll be able to fix this with new.target (only from V8 4.6):

node [vee-eight-4.6●] % more test.js 
'use strict';

class A {
  constructor() {
    console.log(new.target);
  }
}

class B extends A {}

new A();
new B();
node [vee-eight-4.6●] % ./node test
[Function: A]
[Function: B]

@trevnorris
Copy link
Contributor

This still requires that HasInstance() basically becomes an IsUint8Array() check.

@trevnorris
Copy link
Contributor

@domenic @targos Unless I'm missing something, new.target doesn't help class constructors:

'use strict';

class A {
  constructor() {
    if (!new.target)
      return { foo: 42 };
  }
}

let a = A();  // TypeError: Class constructors ...

@trevnorris
Copy link
Contributor

@domenic The inconsistency must come from from mixing ES5 and ES6 style inheritance together. Take this example:

class B extends Uint8Array { constructor() { super(0); } }
class C extends B { constructor() { super(); } }

let c = new C();

console.log(c.constructor == C);
console.log(c.__proto__ == C.prototype);
console.log(c.__proto__.__proto__ == B.prototype);
console.log(c.__proto__.__proto__.__proto__ == Uint8Array.prototype);

But now doing the same thing with Buffer:

class C extends Buffer { constructor() { super(0); } }

let c = new C();

console.log(c.constructor == Buffer);
console.log(c.__proto__ == Buffer.prototype);
console.log(c.__proto__.__proto__ == Uint8Array.prototype);

As you can see it completely dropped anything related to C.

What I'm missing is what new.target could help with.

@domenic
Copy link
Contributor

domenic commented Sep 26, 2015

@trevnorris right now allocate is doing Object.setPrototypeOf(ui8, Buffer.prototype), so even if someone does new C(), you are returning to them something with Buffer.prototype, not C.prototype.

To fix this, you need to pass new.target from the Buffer constructor definition, to allocate, so that allocate can do Object.setPrototypeOf(ui8, newTarget.prototype).

Then the returned object will have the correct prototype. In this case it would be C.prototype, since the target of new in new C() is C.

trevnorris added a commit to trevnorris/node that referenced this issue Oct 6, 2015
Native Buffer method calls do not require anything from the prototype.
So it is unnecessary to check if the Object's prototype is equal to
Buffer.prototype.

This fixes an issue that prevents Buffer from being inherited the ES5
way. Now the following will work:

    function A(n) {
      const b = new Buffer(n);
      Object.setPrototypeOf(b, A.prototype);
      return b;
    }

    Object.setPrototypeOf(A.prototype, Buffer.prototype);
    Object.setPrototypeOf(A, Buffer);

    console.log(new A(4));

Fix: nodejs#2882
trevnorris added a commit that referenced this issue Oct 8, 2015
Native Buffer method calls do not require anything from the prototype.
So it is unnecessary to check if the Object's prototype is equal to
Buffer.prototype.

This fixes an issue that prevents Buffer from being inherited the ES5
way. Now the following will work:

    function A(n) {
      const b = new Buffer(n);
      Object.setPrototypeOf(b, A.prototype);
      return b;
    }

    Object.setPrototypeOf(A.prototype, Buffer.prototype);
    Object.setPrototypeOf(A, Buffer);

    console.log(new A(4));

Fix: #2882
PR-URL: #3080
Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations. question Issues that look for answers.
Projects
None yet
Development

No branches or pull requests

8 participants