Multi-Currency-Checkout: Types #346
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead let the browser handle parsing and rendering of currency amounts with toLocaleString. Also rename 'digits' to 'decimals' for consistency with other code.
sisou
requested changes
Nov 25, 2019
There was a problem hiding this comment.
Too much use of the word undefined for my taste :D
Again, please do not make changes by rebasing! Always add commits when resolving review suggestions. In the end, when it's approved, you can rebase, but not before. Changes are impossible to track with rebasing.
Also, make sure you don't overlook the 53 hidden comments 😉
Comment on lines
+160
to
+162
| || ((this.request as ParsedCheckoutRequest).paymentOptions.find( | ||
| (option) => option.currency === Currency.NIM, | ||
| ) as ParsedNimiqDirectPaymentOptions).protocolSpecific.recipient; |
There was a problem hiding this comment.
I feel this finding of the Nimiq PaymentOption is so much used, it warrants its own helper function. This helper would also be typed correctly, so no need for this repetition everywhere.
Comment on lines
+213
to
+216
| recipient = nimiqPaymentOption.protocolSpecific.recipient!; | ||
| value = nimiqPaymentOption.amount; | ||
| fee = nimiqPaymentOption.protocolSpecific.fee!; | ||
| flags = nimiqPaymentOption.protocolSpecific.flags; |
There was a problem hiding this comment.
Maybe it works here, too? (I have not tried this, just feel this can work.)
{ recipient!, fee!, flags } = nimiqPaymentOption.protocolSpecific;
value = nimiqPaymentOption.amount;
Comment on lines
26
to
+27
| "es2017", | ||
| "esnext", |
There was a problem hiding this comment.
"esnext" includes "es2017", so "es2017" can be removed.
mraveux
requested changes
Nov 26, 2019
nibhar
reviewed
Nov 26, 2019
src/lib/RequestTypes.ts
Outdated
| } | ||
| } | ||
|
|
||
| protected isNonNegativeInteger(value: string | number | bigint | BigInteger) { |
There was a problem hiding this comment.
This seems to me like a function which could move to Helpers.ts.
| } | ||
|
|
||
| public get total(): number { | ||
| return this.amount + this.fee; |
- Fix integer parsing checks for strings - Enforce integer type check and non-negativity for numbers that should be non-negative integers - Add type checking for extraData - Differentiate between undefined and falsish values - Support scientific number notation consistently - Add type checking for nimiq recipient type - Add type checking for nimiq validity duration - Add type checking for nimiq flags - type non-optional parsed NimiqPaymentOption properties as not optional - simplify nimiq address parsing - Avoid code duplication between Nimiq Checkout v1 and v2 parsing - Avoid code duplication for amount parsing
Also updating @types/node to avoid error "Duplicate identifier 'IteratorResult'" between newest typescript and previous @types/node. Also added @types/node to package.json as it was previously only installed as a sub dependency of other dependencies but is actually a direct dependency as we're using node's 'require', 'global' and 'process' at some places in the code. Made it a dev dependency as no node types are required in the output code or types.
Check that the currency and type of the unparsed input payment options object matches the ParsedPaymentOptions it should get parsed to. While for the initial request data this is already ensured by the RequestParser this is relevant for updated data that comes in via the webhook. To access the currency and type of the child classes already in the constructor of the base class, these have been changed from properties to getters. Additionally, some common properties have been moved from the base class to the interface.
danimoh
force-pushed
the
sebastian/checkout/types
branch
from
818fd2c
to
33e401f
Compare
nibhar
force-pushed
the
sebastian/checkout/types
branch
2 times, most recently
from
0698600
to
9391ec1
Compare
also smaller code updates
danimoh
force-pushed
the
sebastian/checkout/types
branch
from
9391ec1
to
1e6f51d
Compare
nibhar
approved these changes
Jan 16, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
First PR in a series of pull requests that add the multi-currency-checkout feature.
This first PR adds the required changes for the checkout API, type definitions and parsing.
Subsequent PRs will add the corresponding UI changes.
Note that this PR is not able to build without these UI changes.
The earliest commit that is able to build is "Update Checkout to use Carousel of CheckoutOptions" in "sebastian/checkout/checkout".
As the multi-currency-checkout PRs combined form the multi-currency-checkout feature, the single PRs target a
multi-currency-checkoutfeature branch which will then be merged intomasteronce all single PRs have been merged together into the feature branch.This way, merge conflicts do not need to be resolved for every single PR but only when merging the complete change set into
masterin the end.