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

ClockDate class overwrite prototype of instance #437

Closed
Oustinger opened this issue Aug 15, 2022 · 10 comments
Closed

ClockDate class overwrite prototype of instance #437

Oustinger opened this issue Aug 15, 2022 · 10 comments

Comments

@Oustinger
Copy link
Contributor

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

  • FakeTimers version : 9.1.2
  • Environment : Opera 89.0.4447.64, Windows 10
  • Example URL : repository
  • Other libraries you are using: -

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
Copy link
Member

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
Copy link
Contributor

fatso83 commented Aug 16, 2022

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
Copy link
Member

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
Copy link
Contributor

fatso83 commented Aug 18, 2022

@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
Copy link
Contributor Author

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

@fatso83
Copy link
Contributor

fatso83 commented Aug 19, 2022

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

@Oustinger
Copy link
Contributor Author

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
Copy link
Contributor

fatso83 commented Aug 19, 2022

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 :)

Copy link

stale bot commented Dec 27, 2023

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.

@stale stale bot added the stale label Dec 27, 2023
fatso83 pushed a commit to Oustinger/fake-timers that referenced this issue Aug 24, 2024
fatso83 added a commit that referenced this issue Aug 24, 2024
…y BREAKING)

See #437 for background. All tests run fine and I have no reason to expect this to do anything but improve things, but it is changing something that has been working for a long time. 

* fix: save methods of children Date instance (#437)

* rename

see pr comment

* fix skipped tests

by ensuring we are checking something meaningful.

* refactor: trim out code that will never be hit in the Proxy

---------

Co-authored-by: Степан Карташев <kartashev@byndyusoft.com>
Co-authored-by: Carl-Erik Kopseng <carlerik@gmail.com>
@fatso83 fatso83 closed this as completed Aug 24, 2024
@fatso83
Copy link
Contributor

fatso83 commented Aug 24, 2024

I fixed up the last few bits of your original PR and merged it. Thank you a bunch for making it! I'll close this and release a version 12 shortly.

github-merge-queue bot pushed a commit to AmadeusITGroup/otter that referenced this issue Sep 13, 2024
## Proposed change

The original issue sinonjs/fake-timers#437 was
fixed in `@sinonjs/fake-timers@>=12`
We get this dependency from `@jest/fake-timers` but it is still using
version 10
It seems that even in Jest 30 the dependency won't be updated to v12

## Related issues

- 🐛 Fixes #(issue)
- 🚀 Feature #(issue)

<!-- Please make sure to follow the contributing guidelines on
https://github.com/amadeus-digital/Otter/blob/main/CONTRIBUTING.md -->
github-merge-queue bot pushed a commit to AmadeusITGroup/otter that referenced this issue Sep 13, 2024
## Proposed change

The original issue sinonjs/fake-timers#437 was
fixed in `@sinonjs/fake-timers@>=12`
We get this dependency from `@jest/fake-timers` but it is still using
version 10
It seems that even in Jest 30 the dependency won't be updated to v12

## Related issues

- 🐛 Fixes #(issue)
- 🚀 Feature #(issue)

<!-- Please make sure to follow the contributing guidelines on
https://github.com/amadeus-digital/Otter/blob/main/CONTRIBUTING.md -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants