Skip to content

Implement Symbol.hasInstance for MockDate #616

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

runjak
Copy link

@runjak runjak commented May 26, 2025

Hey 👋

so I'm enjoying use of both: ladle and date-fns. And this leads me to a situation where I'm using date-fns isValid and isDate to check some data. In particular we've got a component that does date based calculations based on a MockDate.

My understanding is that date-fns depends on the global Date object in code like this:

// node_modules/date-fns/isDate.js
export function isDate(value) {
  return (
    value instanceof Date ||
    (typeof value === "object" &&
      Object.prototype.toString.call(value) === "[object Date]")
  );
}

So when calculating new dates we get instances of RealDate, and check whether a RealDate is instanceof a MockDate, because MockDate rightly replaces the global date when mocking with ladle.

For inheritance this is usually the wrong way: a parent is not an instance of the child class. My suggestions would be to treat it as that though in case of MockDate with the following snippet:

static [Symbol.hasInstance](instance: unknown): boolean {
    return instance instanceof RealDate;
  }

I've tested it by test-wise appending (but not comitting) this code to packages/ladle/lib/app/src/mock-date.ts:

let d = new RealDate();
let m = new MockDate();

console.log({ l: m, d });

console.table([
  ["d instanceof RealDate", d instanceof RealDate],
  ["m instanceof RealDate", m instanceof RealDate],
  ["m instanceof MockDate", m instanceof MockDate],
  ["d instanceof MockDate", d instanceof MockDate],
]);

It leads to output like this:

❯ node packages/ladle/lib/app/src/mock-date.ts
{ l: 2025-05-26T15:36:10.887Z, d: 2025-05-26T15:36:10.887Z }
┌─────────┬─────────────────────────┬──────┐
│ (index) │ 0                       │ 1    │
├─────────┼─────────────────────────┼──────┤
│ 0       │ 'd instanceof RealDate' │ true │
│ 1       │ 'm instanceof RealDate' │ true │
│ 2       │ 'm instanceof MockDate' │ true │
│ 3       │ 'd instanceof MockDate' │ true │
└─────────┴─────────────────────────┴──────┘

Copy link

changeset-bot bot commented May 26, 2025

⚠️ No Changeset found

Latest commit: 6d33f81

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@runjak
Copy link
Author

runjak commented May 28, 2025

@tajo would you like me to add a Changeset to this PR?

@runjak
Copy link
Author

runjak commented Jun 11, 2025

Note that the original MockDate as implemented in https://github.com/boblauer/MockDate/blob/master/src/mockdate.ts#L41 sets the prototype like this:

MockDate.prototype = RealDate.prototype;

With modern JS this leads to the following TypeError:

MockDate.prototype = RealDate.prototype;
                   ^
TypeError: Cannot assign to read only property 'prototype' of function 'class Date extends RealDate {
                         ...<omitted>...
}'

I believe this to be the reason why the prototype line has been omitted from ladles mock date, and it is also what causes the issue in date-fns isDate and when using instanceof between MockDate and Date objects.

That is why I'm asking to adjust the implementation of Symbol.hasInstance.

@tajo
Copy link
Owner

tajo commented Jul 20, 2025

@runjak makes sense. Can you please add some unit test and changeset? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants