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

JSON response can't contain Dates #1800

Closed
DeanMauro opened this issue Dec 10, 2023 · 15 comments · Fixed by #1806
Closed

JSON response can't contain Dates #1800

DeanMauro opened this issue Dec 10, 2023 · 15 comments · Fixed by #1806
Labels

Comments

@DeanMauro
Copy link

What version of Hono are you using?

3.11.4

What runtime/platform is your app running on?

Cloudflare Pages Function

What steps can reproduce the bug?

Return an object containing a Date with context.json. For example:

  const router = new Hono();

  router.get('/test', (context) => {
    const response = {
      dateValue: new Date()
    };

    return context.json(response);
  });

What is the expected behavior?

Because Hono uses JSON.stringify() under the hood, I would expect the code above to be valid.

What do you see instead?

A type error is shown because Date is not considered a JSONObject or JSONPrimitive.

No overload matches this call.
  Overload 1 of 2, '(object: JSONValue, status?: number | undefined, headers?: HeaderRecord | undefined): Response & TypedResponse<never>', gave the following error.
    Argument of type '{ dateValue: Date; }' is not assignable to parameter of type 'JSONValue'.
      Type '{ dateValue: Date; }' is not assignable to type 'JSONObject'.
        Property 'dateValue' is incompatible with index signature.
          Type 'Date' is not assignable to type 'JSONObject | JSONArray | JSONPrimitive'.
            Type 'Date' is not assignable to type 'JSONObject'.
              Index signature for type 'string' is missing in type 'Date'.
  Overload 2 of 2, '(object: JSONValue, init?: ResponseInit | undefined): Response & TypedResponse<never>', gave the following error.
    Argument of type '{ dateValue: Date; }' is not assignable to parameter of type 'JSONValue'.ts(2769)

Additional information

The code executes fine, with the Date being stringified in the response as expected. For this reason especially, I consider the typing to be inaccurate. It disallows a valid type.

@yusukebe
Copy link
Member

Hi @DeanMauro

As you mentioned, indeed the Date object will be converted to a string. However, if we proceed as in #1801, we'll encounter a type mismatch in PRPC mode:

server.ts:

import { Hono } from 'hono'

const app = new Hono().get((c) => c.json({ date: new Date() }))

export default app

client.mts:

import { hc } from 'hono/client'
import type app from 'server'

const client = hc<typeof app>('http://localhost:3000')

const res = await client.index.$get()
const data = await res.json() // { date: Date } <===
console.log(typeof data.date) // string <===

So, I think it not so easy to resolve this issue. And I think it should not allow Date though it uses JSON.stringify().

@ThatOneBro @usualoma Do you have any thoughts?

@AdiRishi
Copy link

I ran into this myself recently on upgrading.

A few thoughts

  • I totally get that it's not semantically correct, and for the library it would be better if the user (us) were responsible for converting the date to it's string representation
  • However, it definitely hurts the usability of the library, especially for the simple cases where we just dump an object from a db query (e.g dirzzle / prisma) into c.json().
  • It is not a simple upgrade path, since we would now either have to ignore the type error or try match behavior with what was previously happening when we stringify our dates

Perhaps one line of code in the release notes to show how to maintain behavior would go a long way.

@yusukebe
Copy link
Member

@AdiRishi Thanks for sharing your thoughts.

However, it definitely hurts the usability of the library, especially for the simple cases where we just dump an object from a db query (e.g dirzzle / prisma) into c.json().

I see, indeed, in that case, the current c.json() is not user friendly. If we can maintain consistency with RPC mode, it might be relaxed for the types of c.json().

@AdiRishi
Copy link

I see, indeed, in that case, the current c.json() is not user friendly. If we can maintain consistency with RPC mode, it might be relaxed for the types of c.json().

Sounds good. Just so I understand, the fix will be to relax the types of c.json so that it accepts dates as valid input. However for RPC mode, the types will keep the same strict behaviour (which makes a lot of sense).

@DeanMauro
Copy link
Author

DeanMauro commented Dec 11, 2023

Thank you both for following up. Perhaps the input and output types should be separated. The output in RPC mode, for example can incorporate a utility type that makes it clear all non-JSONValues have become strings, but without having to put restrictions on the input. E.g.

// Output type
type JSONParsed<T> = {
  [k in keyof(T)]: (T[k] extends JSONValue ? T[k] : string);
}

@yusukebe
Copy link
Member

Hi @DeanMauro

Quick response.

Perhaps the input and output types should be separated.

That makes sense.

@ThatOneBro
Copy link
Contributor

I think letting things get auto-converted to strings fixes types but leads to inconsistent behavior across platforms. Not every runtime will do the same thing for any given runtime object (although Date is likely standard at this point), which makes it a little sketchy for me.

I think we can handle Date as a one-off since it is fairly standard, and under the hood it is not toString being called but toJSON which not every runtime object will have.

What could be done is to allow users to specify a serialize() method for each field, or maybe even a generic one for each type. This is just a quick suggestion but may be very hairy to implement. But I definitely think allowing users to specify serialize, even if it's just (obj) => obj.toString() will prevent users from relying on what could be nonstandard behavior and preventing the code from working crossplatform because of a nonstandard .toString() implementation for a prototype.

@yusukebe
Copy link
Member

@ThatOneBro

Thank you for sharing your thoughts! In this case, since c.json() calls JSON.stringify(), we will make it treat an object as a string.

@DeanMauro @AdiRishi

I've created PR #1806 to fix this issue. Could you review it? The main idea is to separate the input and output types of c.json(), as mentioned by @DeanMauro.

@DeanMauro
Copy link
Author

@yusukebe Thank you for handling this! I appreciate the super fast turnaround :)

@yusukebe
Copy link
Member

This issue has been fixed. You can try the latest version v3.11.5 which includes the fix. Thanks!

@linhtrinh18
Copy link

linhtrinh18 commented Aug 1, 2024

@yusukebe Has it been fixed yet? Im facing this issuse and and looking for solution.
I'm using 4.5.1 version but the Date (backend) automaticly turn to String (at front end) after sending json data through c.json().

@yusukebe
Copy link
Member

yusukebe commented Aug 3, 2024

Hi @linhtrinh18

I'm using 4.5.1 version but the Date (backend) automaticly turn to String (at front end) after sending json data through c.json().

This behavior may be correct, though I don't fully understand your problem.

@austinm911
Copy link

austinm911 commented Aug 18, 2024

Hi @linhtrinh18

I'm using 4.5.1 version but the Date (backend) automaticly turn to String (at front end) after sending json data through c.json().

This behavior may be correct, though I don't fully understand your problem.

I'm getting this problem too so I'll add. My data tables contain dates like createdAt & updatedAt. I'm using Drizzle so the types returned are Dates through my codebase.

So when accessing the hono client everything is typed as a string as mentioned elsewhere in this issue.

// server side
const app = new Hono<{ Bindings: EnvBindings }>()
	.get('/:id', async (c) => {
		const id = Number.parseInt(c.req.param('id'))
		const [data] = await c.env.db.select().from(properties).where(eq(properties.id, id))
		return c.json({ data }) // i.e. createdAt: Date | null;
	})

// client side
	queryFn: async () => {
				if (!id) return
				const res = await client.properties[`:id`].$get({ param: { id: id.toString() } })
				if (!res.ok) {
					throw new Error('Failed to fetch property data')
				}
				return res.json() // i.e. createdAt: string | null;
			},

So because I'm sharing the types inferred by Drizzle in my front end code, I am getting type errors whenever I deal with data returned from the hono RPC client.

What's the recommendation here? I am sure you wouldn't recommend that we cast the type of the data back to the correct type (i.e. the type produced by Drizzle) everywhere we return data from hono RPC? Is there a simple middleware that can be written to do this automatically?

I imagine this would be a common problem, so would be appreciated of your guidance. I'm surprised it's not mentioned elsewhere in the docs.

@grmkris
Copy link

grmkris commented Aug 23, 2024

Hi @linhtrinh18

I'm using 4.5.1 version but the Date (backend) automaticly turn to String (at front end) after sending json data through c.json().

This behavior may be correct, though I don't fully understand your problem.

I'm getting this problem too so I'll add. My data tables contain dates like createdAt & updatedAt. I'm using Drizzle so the types returned are Dates through my codebase.

So when accessing the hono client everything is typed as a string as mentioned elsewhere in this issue.

// server side
const app = new Hono<{ Bindings: EnvBindings }>()
	.get('/:id', async (c) => {
		const id = Number.parseInt(c.req.param('id'))
		const [data] = await c.env.db.select().from(properties).where(eq(properties.id, id))
		return c.json({ data }) // i.e. createdAt: Date | null;
	})

// client side
	queryFn: async () => {
				if (!id) return
				const res = await client.properties[`:id`].$get({ param: { id: id.toString() } })
				if (!res.ok) {
					throw new Error('Failed to fetch property data')
				}
				return res.json() // i.e. createdAt: string | null;
			},

So because I'm sharing the types inferred by Drizzle in my front end code, I am getting type errors whenever I deal with data returned from the hono RPC client.

What's the recommendation here? I am sure you wouldn't recommend that we cast the type of the data back to the correct type (i.e. the type produced by Drizzle) everywhere we return data from hono RPC? Is there a simple middleware that can be written to do this automatically?

I imagine this would be a common problem, so would be appreciated of your guidance. I'm surprised it's not mentioned elsewhere in the docs.

looking for some ideas as well... i pass it through zod parse on client to fix the dates.. but it feels hacky

@austinm911
Copy link

@yusukebe also to note, I had tried to implement the code you linked here #1559 (comment) but ran into type errors as it seems some of the hono types changed after that article was written. It was be appreciated to have an officially recommended way of going about this.

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