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

[Buffer + ES6] Lose the child's class method when extending Buffer in ES6 class style #3739

Closed
hashedhyphen opened this issue Nov 10, 2015 · 8 comments
Assignees
Labels
buffer Issues and PRs related to the buffer subsystem.

Comments

@hashedhyphen
Copy link

This issue is related to #2882 .

I'm now using a nightly version.

>node -v
v5.0.1-nightly201510294e54dbec51

As @trevnorris commented on 25 Sep, a following code doesn't work well now.

'use strict';

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

  foo() {
    console.log('gotcha!');
  }
}

let child = new Child(4);
child.foo();  //=> TypeError: child.foo is not a function

I'm not sure how difficult is this problem to fix, but I would be happy if the above code works...

Thanks! :)

@trevnorris trevnorris added the buffer Issues and PRs related to the buffer subsystem. label Nov 10, 2015
@trevnorris
Copy link
Contributor

Just wrote a conceptual patch to get this working, but haven't been able to remove the performance hit from the current case. Will continue to work on this.

@trevnorris trevnorris self-assigned this Nov 10, 2015
@hashedhyphen
Copy link
Author

@trevnorris Thank you for your quick responce! I'm looking forward to reflecting the patch to a stable version <3

@trevnorris
Copy link
Contributor

For reference, here's the patch and branch of the conceptual fix: trevnorris@c1bc394

@hashedhyphen
Copy link
Author

@trevnorris It is surprising to me that where to fix is JavaScript code. Just looked over c1bc39426d75242da156901a21971c820e2dc686, I wonder if perhaps Object.setPrototypeOf slows the performance as noted in MDN, but I'm not sure...

@trevnorris
Copy link
Contributor

@hashedhyphen The use of Object.setPrototypeOf() is definitely a hit, but we're incurring that cost regardless (since it must be used on Uint8Array() instance). The extra cost comes from accessing new.target. Though my implementation is the first thing that popped out (took most the effort figuring out all the places where it needed to happen) and can probably be easily made faster.

@hashedhyphen
Copy link
Author

@trevnorris I see. Thanks to your kindness, I was able to learn Node's inside more!

@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

Assuming there's no further reason to keep this one open. Closing!

@jasnell jasnell closed this as completed Mar 22, 2016
@techtenk
Copy link

Did the proposed fix above ever go in? I'm running into the same problem on Node v7.10.0

Tims-MBP:nodesandbox techten$ cat buffertest.js
'use strict';

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

  foo() {
    console.log('gotcha!');
  }
}

let child = new Child(4);
child.foo();
Tims-MBP:nodesandbox techten$ node buffertest.js
/Users/techten/Documents/nodesandbox/buffertest.js:14
child.foo();
      ^

TypeError: child.foo is not a function
    at Object.<anonymous> (/Users/techten/Documents/nodesandbox/buffertest.js:14:7)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.runMain (module.js:605:10)
    at run (bootstrap_node.js:427:7)
    at startup (bootstrap_node.js:151:9)
    at bootstrap_node.js:542:3
Tims-MBP:nodesandbox techten$ node -v
v7.10.0

ryzokuken added a commit to ryzokuken/node that referenced this issue Mar 8, 2018
Rename the test appropriately alongside mentioning the subsystem
Also, make a few basic changes to make sure the test conforms
to the standard test structure

Refs: nodejs#19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Mar 11, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the test conforms
to the standard test structure.

This renames:
- test-regress-nodejsGH-1531
- test-regress-nodejsGH-2245
- test-regress-nodejsGH-3238
- test-regress-nodejsGH-3542
- test-regress-nodejsGH-3739
- test-regress-nodejsGH-4256

PR-URL: nodejs#19212
Refs: nodejs#19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Mar 17, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the test conforms
to the standard test structure.

This renames:
- test-regress-GH-1531
- test-regress-GH-2245
- test-regress-GH-3238
- test-regress-GH-3542
- test-regress-GH-3739
- test-regress-GH-4256

PR-URL: #19212
Refs: #19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Mar 20, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the test conforms
to the standard test structure.

This renames:
- test-regress-GH-1531
- test-regress-GH-2245
- test-regress-GH-3238
- test-regress-GH-3542
- test-regress-GH-3739
- test-regress-GH-4256

PR-URL: #19212
Refs: #19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the test conforms
to the standard test structure.

This renames:
- test-regress-nodejsGH-1531
- test-regress-nodejsGH-2245
- test-regress-nodejsGH-3238
- test-regress-nodejsGH-3542
- test-regress-nodejsGH-3739
- test-regress-nodejsGH-4256

PR-URL: nodejs#19212
Refs: nodejs#19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this issue Oct 2, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the test conforms
to the standard test structure.

This renames:
- test-regress-GH-1531
- test-regress-GH-2245
- test-regress-GH-3238
- test-regress-GH-3542
- test-regress-GH-3739
- test-regress-GH-4256

PR-URL: #19212
Refs: #19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
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.
Projects
None yet
Development

No branches or pull requests

4 participants