francescov1 / mongoose-tsgen

A plug-n-play Typescript generator for Mongoose.
105 stars 24 forks source link

Data types for decimal128 are not matching in model and document #105

Closed kawfong closed 2 years ago

kawfong commented 2 years ago

Type-gen for Decimal128 in User is number https://github.com/francescov1/mongoose-tsgen/blob/0bd325755401fbcc4b48e994b79a2dca8a111cfd/src/helpers/tests/artifacts/user.gen.ts#L73

but it's mongoose.Types.Decimal128 in UserDocument https://github.com/francescov1/mongoose-tsgen/blob/0bd325755401fbcc4b48e994b79a2dca8a111cfd/src/helpers/tests/artifacts/user.gen.ts#L202

Is this expected? How do we reserve decimal128 when we want to use the Model?

francescov1 commented 2 years ago

Hi @kawfong, Number seemed like the best choice that Typescript had to offer to represent Decimal128. If Typescript has a more accurate type that we can use, I am happy to change it.

kawfong commented 2 years ago

Why don't we keep it as mongoose.Types.Decimal128? I'm still relatively new to Typescript, I'm not sure what's the best practice here. It seems odd to me that the data types for fields in User are not matching with the data types in UserDocument. The reason that I use Decimal128 instead of number because I need to reserve the floating point precisions for money related fields, converting that between Decimal128 to number will lose the precision. If we have to choose a primitive data type to represent Decimal128, I would prefer string over number

francescov1 commented 2 years ago

@kawfong Every Mongoose model has 2 Typescript types generated, in this case User and UserDocument. The purpose of User is to have a Typescript type which does not have any dependency on Mongoose. This is especially useful for usage in frontends, or any other environment where we don't want to have Mongoose as a dependency.

For general usage of Mongoose documents, you can always use UserDocument, there is no need to use User.

kawfong commented 2 years ago

That makes sense. I'm not sure if string would be a better representation of Decimal128. Coz number basically defeats the purpose of using Decimal128.

Let's say someone is using User to store $1.99 dollar in numberand it gets translated to $1.99 in Decimal128. And this is fine. When someone retrieve it again, it might not be $1.99 as number in User anymore. In terms of reserving precisions, I think string could be a better representation. However, that kinda forces the caller to convert string to some other data types if they ever need to do any operations.

francescov1 commented 2 years ago

I think you're mixing up the purpose of the Typescript type. If a user uses Decimal128, the value in MongoDB will always be a Decimal128. The only purpose that Typescript provides is ensuring type safety when we write code which uses those values. It doesn't actually do any type casting or manipulation of the values themselves at runtime.

So say we get back a Decimal128 from MongoDB. Now, if we decide use the User model to represent that data, Typescript would think that it's number, instead of Decimal128. This is not actually that bad, because it still prevents us from doing any non-number operations on the value. We know for certain that any Decimal128 value is able to be treated like a number without any error. If we instead used string, Typescript would now allow us to treat the value like a string. This is likely to lead to bugs in code, eg we would be able to call value.replace("a", "b") without getting an error from Typescript. When the code actually runs we would get a runtime error.

kawfong commented 2 years ago

Got it. Thank you very much for the explanation.

I'm wondering what's going to happen if someone trying to do operations on the number but it's decimal128 under the hood. Just an incorrect value instead of a runtime error? I'm not sure whether string is better than number or vice versa anymore.

I'll close the comment for now. Thank you for your great work! mongoose-tsgen definitely improves our dev life a lot!

francescov1 commented 2 years ago

I'm not too sure, I haven't worked closely with Decimal128. If you find out more info about it and find it warrants a change here I'm happy to consider it.

Thanks!