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

Dates created before useFakeTimers aren't seen as instanceof Date anymore #504

Closed
erossignon opened this issue Sep 13, 2024 · 6 comments
Closed

Comments

@erossignon
Copy link
Contributor

erossignon commented Sep 13, 2024

  • FakeTimers version : @sinonjs/fake-timers@13.0.1
  • Environment : nodejs
  • Example URL :
  • Other libraries you are using: node-opcua, sinon@19.0.0

What did you expect to happen?

Date object that have been created before fakeTimer takes actions should still be instanceof Date, as it used to be in previous versions

What actually happens

How to reproduce

npm install sinon@19

 import sinon from "sinon";


 // defined before useFakeTimers
 const minDate = new Date(Date.UTC(1601, 0, 1, 0, 0, 0));
 console.log("minDate instanceof Date (before)", minDate instanceof Date);
 const clock = sinon.useFakeTimers();
 const d = new Date();
 console.log("instanceof Date", d instanceof Date);

 console.log("minDate instanceof Date (after) : should be true but is : ", minDate instanceof Date);

 clock.restore();
// without using sinon
import FakeTimers from "@sinonjs/fake-timers";

// emulating what sinonjs is doing:
function createClock(config, globalCtx) {
    let FakeTimersCtx = FakeTimers;
    if (globalCtx !== null && typeof globalCtx === "object") {
        FakeTimersCtx = FakeTimers.withGlobal(globalCtx);
    }
    const clock = FakeTimersCtx.install(config);
    clock.restore = clock.uninstall;
    return clock;
}

 // defined before useFakeTimers
 const minDate = new Date(Date.UTC(1601, 0, 1, 0, 0, 0));
 console.log("minDate instanceof Date (before)", minDate instanceof Date);
 
var clock = createClock();
 const d = new Date();
 console.log("instanceof Date", d instanceof Date);

 console.log("minDate instanceof Date (after) : should be true but is : ", minDate instanceof Date);

 clock.restore();
@erossignon erossignon changed the title Date object created before useFakeTimers aren't seen as instanceof Date anymore Dates created before useFakeTimers aren't seen as instanceof Date anymore Sep 13, 2024
@fatso83
Copy link
Contributor

fatso83 commented Sep 13, 2024

While this is a difference, I am not sure this is a bug or something we should fix up. The change was published as a breaking change, and usually you would install the fake timers before running the tests, run the tests, and then restore things. This would be an issue if you create data used in the tests before installing fake timers, and I am not sure if that's something that is more important to support than supporting instanceof checks on subclasses, which seems like a more real use case to support.

Thoughts?

@erossignon
Copy link
Contributor Author

erossignon commented Sep 13, 2024

also, in some cases, we may have well-defined dates that are global const. (therefore, created before FakeTimer takes action)

We may also need to test that a variant value is of type Date at some point and one of those well-defined date could surface.

̀if (variant instanceof Date) { doSomethingWithDate(variant as Date) }

@fatso83
Copy link
Contributor

fatso83 commented Sep 13, 2024

I did not think it was possible to do anything about this, but Ben and the symbols of ES2015 came to the rescue. You want to test that my proposed fix works for you? #505

I still regard using instanceof as a bit of an anti-pattern in a language as suitable for duck-typing as javascript, but this should satisfy all inclinations 😄

@fatso83
Copy link
Contributor

fatso83 commented Sep 13, 2024

Try the latest 19.0.2 version of Sinon. It should have the fix.

@erossignon
Copy link
Contributor Author

Awesome solution , thanks,
I offer you PR #506 with the missing unit-test.

@fatso83
Copy link
Contributor

fatso83 commented Sep 13, 2024

The test was beautiful, but was already covered: I only linked to the specific commit above that had the fix and the test was in a previous commit. You can see it here:

ae0a266#diff-5a5796b4730f7629082606dc9407d4b8a084ab5ec38803e6141743b4bbb9f7efR3373

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

2 participants