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

Adding all Currencies to also have Currency<bigint> #582

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

reckter
Copy link
Contributor

@reckter reckter commented Feb 8, 2022

Adding an additional 168-bigint to the amendments folder. that contain all currencies in Currency<bigint> format.
I am not sure wether this is the right place or even way of doing it, but as they are currently missing, I thought providing one proposal on how to solve this might be a good idea.

Let me know your thoughts on this, happy to move the folder / or do something completely different :)

@vercel
Copy link

vercel bot commented Feb 8, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/dinerojs/dinerojs/3HYVDrhLGRSPmYLT9TQLJ7HoFCV2
✅ Preview: https://dinerojs-git-fork-opencreek-currencies-bigint-dinerojs.vercel.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 8, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 03e3b1f:

Sandbox Source
@dinero.js/example-cart-react Configuration
@dinero.js/example-cart-vue Configuration
dinerojs/dinero.js Configuration
@dinero.js/example-starter Configuration

Copy link
Contributor

@johnhooks johnhooks left a comment

Choose a reason for hiding this comment

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

I agree, adding BigInt currencies makes sense. Though for packaging as an umd, separating the number and BigInt currencies seems like a better idea to me, just like the calculators.

@sarahdayan
Copy link
Collaborator

sarahdayan commented Nov 4, 2022

Yes, creating two packages in the same fashion as the calculators seems like the right way to go.

@johnhooks
Copy link
Contributor

johnhooks commented Nov 6, 2022

I just created PR #665 it moves the currencies packages to currencies-number. If @sarahdayan thinks this is the way forward, @reckter could you modify this PR to create a new currencies-bigint package?

Also just a though, to avoid duplication, would making a script to generate the currency files be a better approach? All the data could be in a single json file.

@sarahdayan
Copy link
Collaborator

@johnhooks Totally agree that we should generate the currencies. The currency package could actually be repurposed into it, holding a single TypeScript or JSON file with the definitions. Users could reuse it to generate currency packages for custom amount data structure.

@reckter
Copy link
Contributor Author

reckter commented Nov 9, 2022

@johnhooks Looking at #670, it should be fairly trivial with that to implement this PR, so will look into that after it has been merged. (Feel free to just go ahead yourself, If I take to long 😅 ). Thanks for picking this up again 🍺 !

@johnhooks
Copy link
Contributor

Sounds good!

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

Successfully merging this pull request may close these issues.

3 participants