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

Serialization Error: Dates should still be of type Date after useQuery #695

Closed
simonedelmann opened this issue Jun 18, 2020 · 9 comments · Fixed by blitz-js/blitz#827
Closed

Comments

@simonedelmann
Copy link
Contributor

simonedelmann commented Jun 18, 2020

What is the problem?

Prisma returns DateTime fields as Date object in our querys/mutations. Calling these querys/mutations requires to transform the dates to string, as objects cannot be transferred. (#469) This gets done automatically, but I don't know where.

Take this simplified model from Prisma:

model Project {
  id              Int      @default(autoincrement()) @id
  createdAt       DateTime @default(now())
  updatedAt       DateTime @updatedAt
  name            String
  specialDate     DateTime
}

In our pages/components we now recieve a model, for example:

const [project, { mutate }] = useQuery(getProject, { where: { id: projectId } })

project now is of type Project, generated from Prisma. According to this type all DateTime fields (createdAt, updatedAt, specialDate) are provided as Date objects, but actually they are strings.

Steps to Reproduce

  1. Create fresh project blitz new foo; cd foo

  2. Generate model blitz generate all foo name:string

  3. Create a foo instance (via Prisma Studio / generated pages)

  4. Check type of fooDate within /app/foos/querys/getFoo.ts:

const foo = await db.foo.findOne({ where })
console.log(typeof foo.createdAt)    // returns 'object'
  1. Check type of fooDate within /app/foos/pages/foos/[fooId].tsx:
const [foo] = useQuery(getFoo, { where: { id: fooId } })
console.log(typeof foo.createdAt)    // returns 'string'

Versions

debug: global
debug: pkgPath: /Users/simon/.fnm/node-versions/v14.4.0/installation/lib/node_modules/blitz/node_modules/@blitzjs/cli/lib/src/index.js 

macOS Catalina | darwin-x64 | Node: v14.4.0

blitz: 0.15.1 (global)

Suggested solution

To actually use the dates as Date objects, they manually need to be transformed to Date, e.g. foo.createdAt = new Date(foo.createdAt). I think useQuery should do this for us.

@flybayer
Copy link
Member

Yes you're right.

This file and line is where we turn the api call into a json object on the client. We need to convert it back to a date there.

@peaonunes
Copy link
Contributor

peaonunes commented Jun 19, 2020

Hey, correct me if I got that wrong, but I do not think that it is blitz responsibility to do that.

Thinking about querying data from any API you always get the date fields as strings. Specially on rest, if you use fetch, axios, or even useQuery, they will not convert any data type for you. It always returns a string. So it’s common to do some sort of adapting or mapping manually in the frontend if you need to work with Date objects.

Even though we know, in blitz, that they are date objects that knowledge sits in the server side. The types are not carried over the rest request, so when we get the response in the client we do not know the type of the field. I think we cannot convert to the right date object. Inferring the type based on the content would be tricky too 🤔

@simonedelmann
Copy link
Contributor Author

I do get the point. But I don't agree that we should not care about that. I am totally fine converting the dates manually. (Some documentation would be fine, but maybe on Prisma's side.) But we are providing generated code which is somehow wrong:

const [project, { mutate }] = useQuery(getProject, { where: { id: projectId } })

This variable project assumes to be of type Project (generated by Prisma), which assumes dates to be of type Date, which is not the case. If we generate the code, our users will trust, this code works as expected. I think we should take care about that somehow.

Actually Date is the only type provided by Prisma, which causes this error. Dates will always be in this format 2020-09-19T00:00:00.000Z, so it should be possible to detect if a field contains a date and do new Date on that.
If this is not possible / if we don't want that, we can maybe make sure, the variable has the correct type.

@peaonunes
Copy link
Contributor

Yeah, I feel your pain. It's like out of the box blitz is providing a mismatch of types between the type definition that comes from Prisma and our generated code.

Actually Date is the only type provided by Prisma, which causes this error. Dates will always be in this format 2020-09-19T00:00:00.000Z, so it should be possible to detect if a field contains a date and do new Date on that.
If this is not possible / if we don't want that, we can maybe make sure, the variable has the correct type.

That was actually the way I thought about doing it. We would need to use some regex or similar on every field to identify and convert that. Is it possible to have nested fields as well?

Thinking about converting the field to Date, does it eliminate some other operation that the user might want to do and would only be possible with strings? I cannot think of anything right now, but it would be nice to foresee if we were cutting some possibilities out.

I think we could try doing that and if anyone complains (or anyone identifies it is leading to bugs) then we can undo, or revisit it!

@peaonunes
Copy link
Contributor

One thing I was first worried about was the fact that new Date may not be reliable as different browsers implement it differently.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date
image

It's funny that the docs highlight the inconsistency, but do not offer an alternative 🤷

@flybayer
Copy link
Member

flybayer commented Jun 20, 2020

At first I was stumped, but aha, the solution!!

We just need to do more fancy serialization than JSON.parse(JSON.stringify()) which is what we are currently doing.

We could use a some third-party library, but I think we probably want to code our own.


Proposal for Custom Serialization Library

Goals

  1. Serialize to true JSON, not some other syntax. This is important so anyone manually consuming the RPC API can do so without needing a special library to deserialize.
  2. Return the special type information separately so it can be used to reconstruct the original input.
  3. Any Date, Set, Map, and Regexp returned from a Blitz query/mutation will be returned on the client as the same type, not a string.
  4. Be as fast as possible (serialization is often a performance bottleneck)

API

const input = {
  normal: 'string',
  timestamp: new Date(),
  test: /blitz/,
  set: new Set([1, 2]),
  map: new Map([['hello', 'world']])
}

const {json, meta} = serialize(input: any)

/*
json = {
  normal: 'string',
  timestamp: "2020-06-20T04:56:50.293Z",
  test: "/blitz/",
  set: [1, 2],
  map: {
    hello: 'world'
  }
}
// NOTE: `normal` not included here. `meta` only has special cases
meta = {
  timestamp: 'Date',
  test: 'Regexp',
  set: 'Set',
  map: 'Map'
}
*/
const output = deserialize(json, meta)

output <deep equals> input

In addition to result and error, we will add a new meta field to the RPC API. For the above example, the HTTP payload would be:

{
  "result": {
    "normal": "string",
    "timestamp": "2020-06-20T04:56:50.293Z",
    "test": "/blitz/",
    "set": [1, 2],
    "map": {
      "hello": "world"
    }
  },
  "error": null,
  "meta": {
    "specialTypes": {
      "timestamp": "Date",
      "test": "Regexp",
      "set": "Set",
      "map": "Map"
    }
  }
}

I think it makes sense to write this as a standalone serialization library that anyone can use outside of Blitz too.

@flybayer flybayer changed the title Typescript Error: Dates should still be of type Date after useQuery Serialization Error: Dates should still be of type Date after useQuery Jun 20, 2020
@simonedelmann
Copy link
Contributor Author

simonedelmann commented Jun 20, 2020

Thanks, @peaonunes and @flybayer for investigation!
I do like your proposal, it remembers me on Laravels type casting.
We should make it invisible for the user, it should work out of the box. If I understood correctly, your proposal already fulfills this!

@merelinguist
Copy link
Contributor

I’ve opened a PR to create the serializer package. Anyone is welcome to finish this issue by using it in the core package once blitz-js/blitz#707 is merged.

@peaonunes
Copy link
Contributor

Liked a lot the suggestion @flybayer!
Wow, @merelinguist, you have put it together super fast! That's awesome 😄
Happy to work on that once we got the serializer sorted out. Let me know if you need any help with that as well.

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