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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions integration-test/fake-clock-integration-test.js
Expand Up @@ -123,8 +123,8 @@ describe("globally configured browser objects", function () {
now: mockNow,
});

assert.equals(new Date(Date.now()), mockNow);
assert.equals(new Date(), mockNow);
assert.equals(new Date(Date.now()).toString(), mockNow.toString());
assert.equals(new Date().toString(), mockNow.toString());

clock.uninstall();

Expand Down
137 changes: 43 additions & 94 deletions src/fake-timers-src.js
Expand Up @@ -376,109 +376,59 @@ function withGlobal(_global) {
return infiniteLoopError;
}

/**
* @param {Date} target
* @param {Date} source
* @returns {Date} the target after modifications
*/
function mirrorDateProperties(target, source) {
let prop;
for (prop in source) {
if (source.hasOwnProperty(prop)) {
target[prop] = source[prop];
//eslint-disable-next-line jsdoc/require-jsdoc
function createDate() {
class ClockDate extends NativeDate {
/**
* @param {number} year
* @param {number} month
* @param {number} date
* @param {number} hour
* @param {number} minute
* @param {number} second
* @param {number} ms
* @returns void
*/
// eslint-disable-next-line no-unused-vars
constructor(year, month, date, hour, minute, second, ms) {
// Defensive and verbose to avoid potential harm in passing
// explicit undefined when user does not pass argument
if (arguments.length === 0) {
super(ClockDate.clock.now);
} else {
Comment on lines +394 to +398
Copy link
Contributor

@fatso83 fatso83 Feb 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe cover this with a test? 🤔

super(...arguments);
}
}
}

// set special now implementation
if (source.now) {
target.now = function now() {
return target.clock.now;
ClockDate.isFake = true;

if (NativeDate.now) {
ClockDate.now = function now() {
return ClockDate.clock.now;
};
} else {
delete target.now;
}

// set special toSource implementation
if (source.toSource) {
target.toSource = function toSource() {
return source.toSource();
if (NativeDate.toSource) {
ClockDate.toSource = function toSource() {
return NativeDate.toSource();
};
} else {
delete target.toSource;
}

// set special toString implementation
target.toString = function toString() {
return source.toString();
};

target.prototype = source.prototype;
target.parse = source.parse;
target.UTC = source.UTC;
target.prototype.toUTCString = source.prototype.toUTCString;
target.isFake = true;

return target;
}

//eslint-disable-next-line jsdoc/require-jsdoc
function createDate() {
/**
* @param {number} year
* @param {number} month
* @param {number} date
* @param {number} hour
* @param {number} minute
* @param {number} second
* @param {number} ms
* @returns {Date}
*/
function ClockDate(year, month, date, hour, minute, second, ms) {
// the Date constructor called as a function, ref Ecma-262 Edition 5.1, section 15.9.2.
// This remains so in the 10th edition of 2019 as well.
if (!(this instanceof ClockDate)) {
return new NativeDate(ClockDate.clock.now).toString();
}
const ClockDateProxy = new Proxy(ClockDate, {
apply(Target, thisArg, argumentsList) {
// the Date constructor called as a function, ref Ecma-262 Edition 5.1, section 15.9.2.
// This remains so in the 10th edition of 2019 as well.
if (!(this instanceof ClockDate)) {
return new NativeDate(ClockDate.clock.now).toString();
}
Comment on lines +418 to +424
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

had to do a little brush up on Proxy objects and its apply to grok the logic, which made me pay so much attention to the details that I failed to see the toString() bit. That was until I actually read section 15.9.2:

When Date is called as a function rather than as a constructor, it returns a String representing the current time (UTC).

Ooh. Did not remember this. Had to check:

❯ node -p 'Date(2023,01,10)'
Sat Feb 10 2024 11:54:24 GMT+0100 (GMT+01:00)

❯ node -p 'typeof Date(2023,01,10)'
string

I'd rather extract that into a silly, but aptly named, little method called currentDateTimeAsString() and stuff all the details there, though! Makes small minds as my own able to grok the bigger picture faster. Makes for quicker 🆗 on PRs 😅 I know the bit of code was already there, though, so no worries!


// if Date is called as a constructor with 'new' keyword
// Defensive and verbose to avoid potential harm in passing
// explicit undefined when user does not pass argument
switch (arguments.length) {
case 0:
return new NativeDate(ClockDate.clock.now);
case 1:
return new NativeDate(year);
case 2:
return new NativeDate(year, month);
case 3:
return new NativeDate(year, month, date);
case 4:
return new NativeDate(year, month, date, hour);
case 5:
return new NativeDate(year, month, date, hour, minute);
case 6:
return new NativeDate(
year,
month,
date,
hour,
minute,
second
);
default:
return new NativeDate(
year,
month,
date,
hour,
minute,
second,
ms
);
}
}
// if Date is called as a constructor with 'new' keyword
return new Target(...argumentsList);
},
});

return mirrorDateProperties(ClockDate, NativeDate);
return ClockDateProxy;
}

//eslint-disable-next-line jsdoc/require-jsdoc
Expand Down Expand Up @@ -947,8 +897,7 @@ function withGlobal(_global) {
clock[`_${method}`] = target[method];

if (method === "Date") {
const date = mirrorDateProperties(clock[method], target[method]);
target[method] = date;
target[method] = clock[method];
} else if (method === "Intl") {
target[method] = clock[method];
} else if (method === "performance") {
Expand Down
6 changes: 3 additions & 3 deletions test/fake-timers-test.js
Expand Up @@ -3103,7 +3103,7 @@ describe("FakeTimers", function () {
assert(typeof date === "string");
});

it("creates real Date objects when Date constructor is gone", function () {
it.skip("creates real Date objects when Date constructor is gone", function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test does not seem very useful. I would check the Git history for references.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dug back, found it was just copied over from Sinon ten years ago, so can't be bothered to look deeper:
Oustinger@dbd7d17#diff-7c9e5e3f3dd46601b73d2d05be43d6d7a33f2f7ccd564494f4eb7b3f4ae915e0R636

The only meaningful bit here about the test is that we check that everything still works as before. So the result of the toString should be the same before and after the removal of the global Date.

const realDate = new Date();
Date = NOOP; // eslint-disable-line no-global-assign
global.Date = NOOP;
Expand Down Expand Up @@ -3221,7 +3221,7 @@ describe("FakeTimers", function () {
assert.equals(fakeDateStr, new this.clock.Date().toString());
});

it("mirrors native Date.prototype", function () {
it.skip("mirrors native Date.prototype", function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could change this to be a test that it passes an instanceof check. it("is a Date instance", ...?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just kill the test. It would just be this in the end:

node -p 'class FooDate extends Date {}; D=Date; global.Date = function(){};  (new FooDate()) instanceof D'
true

assert.same(this.clock.Date.prototype, Date.prototype);
});

Expand Down Expand Up @@ -3283,7 +3283,7 @@ describe("FakeTimers", function () {
});

it("mirrors toString", function () {
assert.same(this.clock.Date.toString(), Date.toString());
fatso83 marked this conversation as resolved.
Show resolved Hide resolved
assert.same(this.clock.Date.toString, Date.toString);
});
});

Expand Down
29 changes: 29 additions & 0 deletions test/issue-437-test.js
@@ -0,0 +1,29 @@
"use strict";

const { FakeTimers, assert } = require("./helpers/setup-tests");

describe("issue #437", function () {
it("should save methods of children instance", function () {
fatso83 marked this conversation as resolved.
Show resolved Hide resolved
const clock = FakeTimers.install();

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

this.bar = "bar";
}

foo() {
return "Lorem ipsum";
}
}

const dateTime = new DateTime();

// this would throw an error before issue #437 was fixed
assert.equals(dateTime.foo(), "Lorem ipsum");
assert.equals(dateTime.bar, "bar");

clock.uninstall();
});
});