-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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(remix-server-runtime): fix Jsonify
for objects with toJSON
function
#7736
base: dev
Are you sure you want to change the base?
Conversation
|
Expect<Equal<Pretty<Jsonify<{a: [1, "two", Date, undefined, false]}>>, {a: [1, "two", string, null, false]}>>, | ||
Expect<Equal<Pretty<Jsonify<{a: [unknown, 1]}>>, {a: [unknown, 1]}>>, | ||
// @ts-expect-error | ||
Expect<Equal<Pretty<Jsonify<{a: MyClass}>>, {a: {a: string}}>>, |
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.
I can't seem to get these nested classes to work
@pcattori probably has a good idea where to look at
7a448d1
to
8877f1e
Compare
@@ -160,14 +167,31 @@ type _tests = [ | |||
|
|||
// object | |||
Expect<Equal<Pretty<Jsonify<{}>>, {}>>, | |||
// @ts-expect-error | |||
Expect<Equal<Pretty<Jsonify<{a: any}>>, {a: any}>>, |
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.
I also don't really get why this one is failing tbh 🤔
If I change this to be equal to {a?: any}
, this passes though
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.
@MichaelDeBoey could you split this up into smaller, more targeted PRs?
Expect<Equal<Jsonify<{ toJSON(): Date }>, unknown>>, | ||
Expect<Equal<Jsonify<{ toJSON(): any }>, any>>, | ||
Expect<Equal<Jsonify<{ toJSON(): undefined }>, never>>, | ||
Expect<Equal<Jsonify<{ toJSON(): Date }>, string>>, |
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.
Haven't checked the other test cases, but this one is incorrect.
let result = JSON.parse(JSON.stringify({ toJSON: () => new Date() }))
// => {}
So the result is an empty object, not a string
. toJSON
doesn't seem to always recursively serialize the return value.
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.
I was checking, which did respond with a string
let result = JSON.parse(JSON.stringify({ toJSON: () => Date() }))
// => 'Mon Oct 23 2023 03:45:02 GMT+0200 (Midden-Europese zomertijd)'
@pcattori Can you explain how you would see the separate PRs?
|
8877f1e
to
5aac416
Compare
I also added some extra tests for objects
Closes #7821