-
Notifications
You must be signed in to change notification settings - Fork 95
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
create new types for createOrder and creatVaultToken #605
Conversation
🦋 Changeset detectedLatest commit: 35591cd The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall.
|
||
export interface PayPalCardFieldsComponentCreateVaultSetupToken | ||
extends PayPalCardFieldsComponentBasics { | ||
createOrder?: never; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets revisit if we need createOrder?: never;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if createOrder?: never
add any value, so i feel like we can remove that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be value in future clarity.
@hnguyen510 can you also add |
expect(createVaultSetupTokenCallback.createOrder).toBeUndefined(); | ||
}); | ||
|
||
test.skip("Can't have both", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these test still supposed to be skipped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still need to be skip right now because there is assertionError. Not sure what assertion call I should do instead.
|
||
export interface PayPalCardFieldsComponentCreateVaultSetupToken | ||
extends PayPalCardFieldsComponentBasics { | ||
createOrder?: never; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be value in future clarity.
.changeset/silly-bears-boil.md
Outdated
@@ -0,0 +1,5 @@ | |||
--- | |||
"@paypal/paypal-js": major |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is probably a minor
change since its not breaking any existing usage of the sdk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the changeset, how do i remove the old one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can delete this file
This pr is to fix this LI-73058
Problem: the createOrder callback is require when createVaultToken is implement when it should be one or the other and not both.
Closes #587
Closes #580