kossnocorp / typesaurus

🦕 Type-safe TypeScript-first ODM for Firestore
https://typesaurus.com
415 stars 36 forks source link

Possibility to disable timestamps being converted to dates #113

Closed daniel closed 2 years ago

daniel commented 2 years ago

I've stumbled upon a problem with Typesaurus automatically converting timestamps to dates. In cloud functions background triggers, e.g. "on create" I get the data with timestamps, but when I use Typesaurus to fetch data I get the data with dates. This means that my type definition must declare that the date can be either timestamp or date. This also adds complexity that any code that uses the data - it needs to check which type the date is and sometimes convert it.

Example type:

export type Example {
  title: string
  createdAt?: FirebaseFirestore.Timestamp | Date
}

I think it would be simpler if typesaurus never converted timestamps to dates, so I would like the possiblity to disable it.

I can't find where in the code this conversion is done, but if you could point me in the write direction I maybe could implement or patch it myself.

Or maybe there is a better solution for my problem?

SrBrahma commented 2 years ago

Hi @daniel. I've implemented https://github.com/kossnocorp/typesaurus/pull/111 few days ago on top of the v8 branch, a newer one, to implement settings. You could add something like convertTimestampToDate as setting, defaulting to true to keep the current behaviour.

What you want to change is here, in wrapData function:

https://github.com/kossnocorp/typesaurus/blob/7ec7091da299592751bca055bc7158e0d8ccaa2a/src/data/index.ts#L63-L64

What I've done in my linked PR is

else if (data === undefined && Settings.undefinedToNull)

So, your code could just add && Settings.convertTimestampToDate on the Timestamp conditional.

I've been in contact with the package author, and he will eventually make some updates, certainly accepting #111 and yours. While he doesn't and if you want, you can PR it on top of v8 branch and I will add it to my current fork, containing other updates, mentioned here: https://github.com/kossnocorp/typesaurus/issues/110#issuecomment-1008353919.

daniel commented 2 years ago

Thanks! I tried forking typesaurus and making some changes, but I couldn't figure out how to make the package build when installed in another project. I've tried adding a prepare script inside package.json, but weren't able to make it work. Do you know how to do that?

kossnocorp commented 2 years ago

I'm not sure if it's worth adding this option. It will make types much harder to define and resolve (for TypeScript).

I propose to have a wrapper for Functions triggers that wrap the data correctly. There's a need for it anyway.

Unless I'm missing something and they're more cases when timestamps get exposed, I prefer keeping the date behavior as is and adding Functions wrapper instead.

SrBrahma commented 2 years ago

Maybe @daniel can even use the wrapData function on the trigger data to convert the Timestamps to Dates, as an existing solution.

Answering your question, Daniel, what I've been doing to have my forks installed on my project is: first I created an empty lib branch on another dir, then I build the project, move the new /lib content to the lib branch dir and commit it. That's why my fork install command have a #lib on its end. Not sure if there is a better way that fits this package dir structure, but it works.

SrBrahma commented 2 years ago

@daniel, another user here days ago mentioned a trigger lib, https://github.com/kossnocorp/typesaurus/issues/110#issuecomment-1008436268, the https://github.com/Yuiki/typed-ff.

For my surprise, it not only uses typesaurus but also uses that wrapData idea I had. It should exactly fit your case 😉

I was considering before noticing it to implement those triggers on typesaurus, but as the linked lib already does it, I don't think it would worth the work.

daniel commented 2 years ago

Thanks @kossnocorp and @SrBrahma ! Yesterday I made a simple wrapper for function triggers and it solved my problem. I'm gonna take a look typed-ff, that's probably an even better solution.

kossnocorp commented 2 years ago

@daniel, I'm glad that it worked out for you. I contacted the author of typed-ff and suggested to join the efforts: https://github.com/Yuiki/typed-ff/issues/8

I have a few ideas on adding more type safety to the thing. For instance, the custom id isn't safe, but there's a way to make it happen.

I'm closing the issue as there's nothing actionable at the moment.