Skip to content

Can't call functions of classes derived from built-in classes #113

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

Closed
harmony7 opened this issue Jun 17, 2022 · 4 comments
Closed

Can't call functions of classes derived from built-in classes #113

harmony7 opened this issue Jun 17, 2022 · 4 comments
Assignees

Comments

@harmony7
Copy link
Member

harmony7 commented Jun 17, 2022

Consider the following code that works as expected:

class Base {
  fn() { return 'fn'; }
}
class Derived extends Base {
  constructor() {
    super();
    this.foo = () => { return 'foo'; };
  }
  bar() { return 'bar'; }
  get baz() { return 'baz'; }
}

const obj = new Derived();
console.log(Object.getOwnPropertyNames(Base.prototype));    // constructor,fn
console.log(Object.getOwnPropertyNames(Derived.prototype)); // constructor,bar,baz
console.log(Object.getOwnPropertyNames(obj));               // foo
console.log(obj.fn());                                      // fn
console.log(obj.foo());                                     // foo
console.log(obj.bar());                                     // bar
console.log(obj.baz);                                       // baz
console.log(obj.foo === undefined);                         // false
console.log(obj.bar === undefined);                         // false
console.log(obj.baz === undefined);                         // false

When you try to do the same thing by extending builtins:

class MyRequest extends Request {
  constructor(input, init) {
    super(input, init);
    this.foo = () => { return 'foo'; };
  }
  bar() { return 'bar'; }
  get baz() { return 'baz'; }
}

const request = new MyRequest('https://www.google.com/');
console.log(Object.getOwnPropertyNames(Request.prototype));   // constructor,method,url,version,headers,body,bodyUsed,arrayBuffer,json,text,setCacheOverride
console.log(Object.getOwnPropertyNames(MyRequest.prototype)); // ** constructor,bar,baz
console.log(Object.getOwnPropertyNames(request));             // foo
console.log(request.url);                                     // https://www.google.com/
console.log(request.foo());                                   // foo
console.log(request.bar());                                   // ** Error: request.bar is not a function
console.log(request.baz);                                     // ** undefined
console.log(request.foo === undefined);                       // false
console.log(request.bar === undefined);                       // ** true
console.log(request.baz === undefined);                       // ** true

Look at the logs relating to bar and baz (marked with asterisks above). You can see that the two methods (bar method and baz getter) added through the class definition are added to the prototype but have undefined values. Note that the property added in the constructor (foo) does work, however.

The same problem happens whether we are extending Request, Response, Headers, URL, URLSearchParams, etc. so I believe it is just for the builtins, and I believe it's a bug.

@JakeChampion
Copy link
Contributor

I looked into this and can confirm the base prototype is being used and not the extended prototype.
I believe I know what the issue and the fix would be.
We currently use JS_NewObjectWithGivenProto to create instances but that doesn't do anything to work out what the callee was. We should be using the JS_NewObjectForConstructor to construct instances of our extendable classes such as Request as JS_NewObjectForConstructor will use the callee to determine parentage and [[Prototype]], which allows applications to extend from Request.
I can take on this work for all our built-in classes 👍

@JakeChampion
Copy link
Contributor

@harmony7 I have opened a pull-request which should resolve this for Request -- #116

We can raise separate pull-requests for the other built-in classes which can use the same approach as the Request implemention 👍

@jameysharp
Copy link
Contributor

I believe this issue has been fixed with the release of version 0.3.0, which is now on npm. Thank you for reporting it!

@harmony7
Copy link
Member Author

so happy to hear that, I'm going to try it out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants