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

feat: add support for float properties #10551

Merged
merged 5 commits into from
Dec 12, 2024
Merged

Conversation

thetutlage
Copy link
Contributor

@thetutlage thetutlage commented Dec 11, 2024

Fixes: FRMW-2844

This PR introduces the support for float values using DML. Under the hood, the value is stored as numeric with PostgreSQL and converted to a number using the Number serializer.

The float values can be used to represent values with decimal precision. For example:

const tax = model.define("tax", {
  id: model.number(),
  rate: model.float()
})

@thetutlage thetutlage requested a review from a team as a code owner December 11, 2024 12:12
Copy link

vercel bot commented Dec 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
api-reference 🔄 Building (Inspect) Visit Preview 💬 Add feedback Dec 11, 2024 4:11pm
medusa-dashboard 🛑 Canceled (Inspect) Dec 11, 2024 4:11pm
5 Skipped Deployments
Name Status Preview Comments Updated (UTC)
api-reference-v2 ⬜️ Ignored (Inspect) Visit Preview Dec 11, 2024 4:11pm
docs-ui ⬜️ Ignored (Inspect) Visit Preview Dec 11, 2024 4:11pm
docs-v2 ⬜️ Ignored (Inspect) Visit Preview Dec 11, 2024 4:11pm
medusa-docs ⬜️ Ignored (Inspect) Visit Preview Dec 11, 2024 4:11pm
resources-docs ⬜️ Ignored (Inspect) Visit Preview Dec 11, 2024 4:11pm

Copy link

changeset-bot bot commented Dec 11, 2024

🦋 Changeset detected

Latest commit: c40d4b8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 65 packages
Name Type
@medusajs/types Patch
@medusajs/utils Patch
@medusajs/dashboard Patch
integration-tests-http Patch
@medusajs/cli Patch
@medusajs/medusa Patch
@medusajs/test-utils Patch
@medusajs/api-key Patch
@medusajs/auth Patch
@medusajs/cache-inmemory Patch
@medusajs/cache-redis Patch
@medusajs/cart Patch
@medusajs/currency Patch
@medusajs/customer Patch
@medusajs/event-bus-local Patch
@medusajs/event-bus-redis Patch
@medusajs/file Patch
@medusajs/fulfillment Patch
@medusajs/index Patch
@medusajs/inventory Patch
@medusajs/link-modules Patch
@medusajs/locking Patch
@medusajs/notification Patch
@medusajs/order Patch
@medusajs/payment Patch
@medusajs/pricing Patch
@medusajs/product Patch
@medusajs/promotion Patch
@medusajs/region Patch
@medusajs/sales-channel Patch
@medusajs/stock-location Patch
@medusajs/store Patch
@medusajs/tax Patch
@medusajs/user Patch
@medusajs/workflow-engine-inmemory Patch
@medusajs/workflow-engine-redis Patch
@medusajs/auth-emailpass Patch
@medusajs/auth-github Patch
@medusajs/auth-google Patch
@medusajs/file-local Patch
@medusajs/file-s3 Patch
@medusajs/fulfillment-manual Patch
@medusajs/locking-postgres Patch
@medusajs/locking-redis Patch
@medusajs/notification-local Patch
@medusajs/notification-sendgrid Patch
@medusajs/payment-stripe Patch
@medusajs/core-flows Patch
@medusajs/framework Patch
@medusajs/js-sdk Patch
@medusajs/modules-sdk Patch
@medusajs/orchestration Patch
@medusajs/workflows-sdk Patch
@medusajs/medusa-oas-cli Patch
@medusajs/oas-github-ci Patch
@medusajs/telemetry Patch
@medusajs/admin-bundler Patch
@medusajs/admin-sdk Patch
@medusajs/admin-shared Patch
@medusajs/admin-vite-plugin Patch
@medusajs/icons Patch
@medusajs/toolbox Patch
@medusajs/ui-preset Patch
create-medusa-app Patch
medusa-dev-cli Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

*/
if (field.dataType.name === "float") {
Property({
columnType: "numeric",
Copy link
Contributor

@olivermrbl olivermrbl Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: shouldn't this be real or double precision to mirror Postgres?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double checked it and it seems that you can loose some precision with real and double. For example: If I run the following query where the tax_rate_real is a REAL column, then the updated value will be 0.102111116.

UPDATE "public"."foo" SET "tax_rate_real" = 0.102111114;

Ofcourse, in our case we don't need these many decimal places, but will be nice to cover them for other use-cases with a more accurate data-type. WDYY?

Copy link
Contributor Author

@thetutlage thetutlage Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CleanShot 2024-12-11 at 19 53 38@2x

Copy link
Contributor

@olivermrbl olivermrbl Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I thought the argument for not using the bigNumber column here and introducing float was that we didn't care about high precision for tax rates. If we want to solve for the scenario you describe, would it not be more appropriate to just use bigNumber?

Copy link
Contributor Author

@thetutlage thetutlage Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am okay with using REAL. Have no strong arguments using numeric.

The issue with bigNumber was its representation within the JavaScript runtime. Not having the value next to the field and having it inside the _raw property as an object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 👍

Copy link
Contributor

@olivermrbl olivermrbl Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me!

Just to be clear, I think your argument for using numeric makes sense–it offers higher precision for the rate of tax rates. That said, we introduced the bigNumber property specifically for these use cases and I'd like us to continue using this whenever relevant to ensure consistency and leverage our existing utilities. That’s why I pushed back on adding a new type with somewhat similar characteristics. It’s not that high precision isn’t valid. I hope this rationale makes sense.

Let's go with real now and we can revisit later if it leads to issues.

@thetutlage thetutlage merged commit 885c82d into develop Dec 12, 2024
23 checks passed
@thetutlage thetutlage deleted the feat/dml-float-datatype branch December 12, 2024 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants