performance: fix timerify bug

[iam-frankqiu]

Fix #40623

[benjamingr]

This would fail if someone is using "old" classes which can be common when transpiling cross-platform code that targets old targets (for example: using tooling that uses es5 at any point)

[iam-frankqiu]

This would fail if someone is using "old" classes which can be common when transpiling cross-platform code that targets old targets (for example: using tooling that uses es5 at any point)

Maybe you can add a test on test-performance-function.js or just show me an example function(I will add to the former file). I will solve the problem to ensure all tests have been passed.

[benjamingr]

Sure

function MyClass {
  this.foo = 15;
}
MyClass.prototype.sayFoo {
  console.log(this.foo);
}
const instance = new MyClass(); ?

[iam-frankqiu]

this.foo = 15;
Hasn't it a return value ?

[iam-frankqiu]

Sure

function MyClass {
  this.foo = 15;
}
MyClass.prototype.sayFoo {
  console.log(this.foo);
}
const instance = new MyClass(); ?

If a function is like this. Actually, we can't judge which way is to call. Because using new to call and invoke directly is different. So I think we just invoke it directly if the first augment is a function. Using new to call if it is a class. wdyt?

[benjamingr]

I think that will not work for a large chunk of our ecosystem unfortunately and there are people who still use "old style" constructors or have existing projects that do it. So I think for me personally this not breaking for "old style" classes is important.

[targos]

Maybe we should change the check from "is it a constructor?" to "is it called as a constructor?". i.e. new.target !== undefined. wdyt?

[benjamingr]

That sounds like a very reasonable approach to me

[benjamingr]

Note new.target can be set but the return value can still be overridden by returning something different - so a constructor check is would still need to check the return value

[iam-frankqiu]

Maybe we should change the check from "is it a constructor?" to "is it called as a constructor?". i.e. new.target !== undefined. wdyt?

Actually. I have considered this problem. It seems like new.target only can be used within a function. I didn't find any way to use it outside of a function.

[targos]

It seems like new.target only can be used within a function. I didn't find any way to use it outside of a function.

This should be fine. timerified is a function.

[iam-frankqiu]

It seems like new.target only can be used within a function. I didn't find any way to use it outside of a function.

This should be fine. timerified is a function.

I think we don't need to judge if it is called a constructor. we only judge whether the argument is a function or not. if is a class we use new to call, else we invoked directly.

[targos]

I don't agree. In my opinion, timerified should be a proxy to the function and behave as much as possible the same as if the function was called directly.

[iam-frankqiu]

I don't agree. In my opinion, timerified should be a proxy to the function and behave as much as possible the same as if the function was called directly.

There must have been some misunderstanding in there. What I do is just like you said. Are there any conflicts?

[aduh95]

The conflict is that, in JavaScript, a function can be a class. What Michaël is saying is that, no matter what we can infer of the argument being a "class" or not, if timerified is invoke using new keyboard (i.e. if new.target !== undefined), we should pass that along.

[aduh95]

Suggested change

	const result = isCalledAsConstructor(fn) ?
	ReflectConstruct(fn, args, fn) : ReflectApply(fn, this, args);
	const result = new.target !== undefined ?
	ReflectConstruct(fn, args, fn) :
	ReflectApply(fn, this, args);

[aduh95]

You marked this conversation as resolved but didn't take my suggestion. Is it because you disagree with it, or because you forgot to push?

[RaisinTen]

@aduh95 wouldn't new.target not be undefined only if timerified() is called with new instead of fn()?

[targos]

Yes, but that's the point. We want the timerified function to behave the same as the original function in both cases.

[RaisinTen]

Ah, we are returning timerified from timerify. Yea, this makes sense 👍

[benjamingr]

Leaving a review so this doesn't land by accident before we have consensus on the semantics - I feel strongly that this should work for "old style" classes before we merge and that we should address the fact that classes can have return value (and a test) - but I am ready to be convinced otherwise.

[iam-frankqiu]

Leaving a review so this doesn't land by accident before we have consensus on the semantics - I feel strongly that this should work for "old style" classes before we merge and that we should address the fact that classes can have return value (and a test) - but I am ready to be convinced otherwise.

Ok. If a function like below:

function MyClass () {
  this.foo = 15;
  return 20
}
MyClass.prototype.sayFoo = () => {
  console.log(this.foo);
}
new MyClass()
> MyClass {foo: 15}
MyClass()
> 20

If this function is a parameter to timerify. I really don't know we should use Reflect.constructor or
Reflect.apply to execute it. I think that is the question. Who can tell me how to do? @benjamingr @targos That's what I want to say. Thank you.

[iam-frankqiu]

I think maybe we can add a comment to the parameter to tell users. This is the only way I can think of to solve the question. This is a bug and we need to solve it. How about this way? @benjamingr @targos

[aduh95]

I still think #40625 (comment) would be a better solution, and would align with what Benjamin and Michaël are asking. Is there a reason you don't want to implement it?

[hax]

I think we should usenew.target trick. Is there any other reason why this PR not landed?

[cola119]

@iam-frankqiu Any updates? If not, I'd like to create another PR that fixes this issue with new.target way.

[aduh95]

Superseded by #43330.