sinonjs / fake-timers

Fake setTimeout and friends (collectively known as "timers"). Useful in your JavaScript tests. Extracted from Sinon.JS
BSD 3-Clause "New" or "Revised" License
794 stars 103 forks source link

ClockDate class overwrite prototype of instance #437

Open Oustinger opened 1 year ago

Oustinger commented 1 year ago

ClockDate class overwrite prototype of instance. In my real project it happens when I use datepicker library (@eonasdan/tempus-dominus).

What did you expect to happen? I expected to receive instance of DateTime with it's methods

What actually happens Prototype of child instance is not equals to DateTime class, but equals to fake date class

How to reproduce I'd created a repository with reproducing this problem

i. You can clone and run project ii. Or you can create project with @sinonjs/fake-timers and run the code below by yourself

var FakeTimers = require("@sinonjs/fake-timers");

FakeTimers.install();

class DateTime extends Date {
    constructor() {
        super();

        this.bar = 'bar';
    }

    foo() {
        console.log('Lorem inpsum');
    }
}

var dateTime = new DateTime();

dateTime.foo();
benjamingr commented 1 year ago

This is probably a bug because when you new something in JS and return something different from the constructor - it will return that thing.

class Foo {
  constructor() {
     return { x: 5 };
  }
  bar() {} 
}
const f = new Foo();
f.bar(); 

We should probably fix the code to not do that or call Reflect.construct(new.target) instead of assuming we have a NativeDate.

fatso83 commented 1 year ago

when you new something in JS and return something different from the constructor

Where do we do that? I could not see this happening in the example code.

benjamingr commented 1 year ago

Where do we do that? I could not see this happening in the example code.

Our ClockDate function doesn't use this and instead has a return value even for the case it doesn't return a native date.

fatso83 commented 1 year ago

@Oustinger The quickest path to seeing a patch commit for this is probably submit a small PR with a fix and regression test. I can make a release quick, but I do not have time atm to debug, fix and verify this.

Oustinger commented 1 year ago

@fatso83 I would do that. But I couldn't find a place to fix on my own.

fatso83 commented 1 year ago

No one would expect you to 🙂 I certainly did not. Do you think you have the hints you need?

Oustinger commented 1 year ago

Where is a place in code, where new date's prototype becomes equal to ClockDate? My debugger can't go deeper this line. What is going on after that?

fatso83 commented 1 year ago

You cannot get deeper than that function, but the setup of the prototype happens before that

https://github.com/sinonjs/fake-timers/blob/main/src/fake-timers-src.js#L446-L489 covers all the return bits where we create NativeDate instances and line 491 is where return our own mirrored constructor. That function call sets up the prototype.

We should probably fix the code to not do that

Not totally sure how to fix the code myself TBH. ~I am guessing we are invoking NativeDate to create Date objects that are identical in behavior to the NativeDate, and if we were not to invoke NativeDate directly I am not sure what to do. ~

This fixed the issue when I tested:

                        //return new NativeDate(ClockDate.clock.now);
                        NativeDate.call(this,ClockDate.clock.now);
                        return;

Not sure if this is a general fix, but try and see if the test suite works when doing it for all paths :)

stale[bot] commented 6 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.