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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Ooh. Did not remember this. Had to check:
I'd rather extract that into a silly, but aptly named, little method called |
||
|
||
// 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 | ||
|
@@ -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") { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: 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; | ||
|
@@ -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 () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just kill the test. It would just be this in the end:
|
||
assert.same(this.clock.Date.prototype, Date.prototype); | ||
}); | ||
|
||
|
@@ -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); | ||
}); | ||
}); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
"use strict"; | ||
|
||
const { FakeTimers, assert } = require("./helpers/setup-tests"); | ||
|
||
describe("issue #437", function () { | ||
it("should save methods of subclass instance", function () { | ||
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(); | ||
}); | ||
}); |
There was a problem hiding this comment.
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? 🤔