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

Support Prisma's Decimal field in our serialisation #2503

Open
infomiho opened this issue Feb 12, 2025 · 2 comments
Open

Support Prisma's Decimal field in our serialisation #2503

infomiho opened this issue Feb 12, 2025 · 2 comments
Labels

Comments

@infomiho
Copy link
Contributor

Prisma has a Decimal type which SuperJSON can't handle, we need to implement custom handling for it.

We should probably implement a custom serializer for the Prisma.Decimal type: https://github.com/flightcontrolhq/superjson?tab=readme-ov-file#decimaljs--prismadecimal

Source: https://ptb.discord.com/channels/686873244791210014/1337972417171165296/1338461339898740757

@infomiho infomiho added bug Something isn't working prisma shouldfix We should do/fix this at some point labels Feb 12, 2025
@sodic
Copy link
Contributor

sodic commented Feb 12, 2025

Hm, this isn't a bug. It's probably an enhancement (assuming we do it).

That said, I'm not sure we should do it. Perhaps we should approach it from a different angle (e.g., better support for custom serialization).

Decimal is an some arbitrary database type (probably a class of some sort). JavaScript doesn't support it natively, and Superjson is a serialization layer for native JavaScript types.

I understand the convenice of directly returning the result of a Prisma operation from a Query**, but supporting its serialization seems out of place (imagine Decimal sitting in this table).
Why stop at Decimal, we should then support all custom Prisma types. And even then, why stop at Prisma? We should support all possible custom types anyone can come up with. We have to draw the line somewhere, and I feel Superjson drew it in the right place.

We could add support for generic clases though (Superjson supports kinda supports it: flightcontrolhq/superjson#34), but I think there was a type-safety issue with that.

Another relevant issue: #2027
@infomiho I've changed some labels, I think these are more appropriate.

** Btw, I know we do this a lot in our examples, but directly returning Prisma results isn't the best of practices. It makes it really easy to accidentally leak suff to the client.

@sodic sodic added enhancement New feature or request discussion and removed bug Something isn't working shouldfix We should do/fix this at some point labels Feb 12, 2025
@infomiho
Copy link
Contributor Author

And even then, why stop at Prisma? We should support all possible custom types anyone can come up with.

I have to counter this by saying: we chose to build a batteries included framework with Prisma at its core - so it's not going too far if we want to support serializing Prisma results in our queries. Users will hit this - we should have a solution for them, either automatically (serialisation upgrade) or manually (with some helpers that transform the problematic data types).

I wanted to mention #2027 originally, but I felt like these are separate issues (one is about upgrading what we already have, the other will require research to see if it will be possible to build it and if we want to build it).

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

No branches or pull requests

2 participants