-
Notifications
You must be signed in to change notification settings - Fork 563
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
Defining ThreeDomainSecureClient component interface #2447
Conversation
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 Great!!!
currencyCode: string, | ||
billingCountryCode: string, | ||
amount: string, | ||
includeBuyerInstallments?: boolean, |
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.
nit: do we want to remove the lint updates out of this PR for clarity?
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.
Good call. I can create a separate PR for the lint commits.
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.
We can leave it in for this one. But also the comment, so it is easier to trace back that it is simply a lint update and not actual code changes.
vi.mock("../lib", () => ({ | ||
ValidationError: vi.fn(), | ||
})); | ||
describe("getThreeDomainSecure returns ThreeDomainSecureComponent", () => { |
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.
Do we still want to keep this test?
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 moved this test further down on Line 46 as a general initialization test. Instead of mocking the ValidationError I'm just asserting that it would throw an error. Is this OK or does it change the test? I wasn't entirely sure.
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 see, yeah that should work too. My idea with that test was to capture validationError
specifically since it will be one of the trace events in the future.
This is not a blocker for the PR to be merged
|
||
return ThreeDomainSecureAuth; | ||
type SdkConfig = {| | ||
sdkToken: ?string, |
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.
nit: We can probably remove the ?
here. It is required to be a string
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.
Agreed!
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.
Removing this creates a flow error. I believe that it is possible for null or undefined to be returned by getSDKToken
which is why it should be optional
Description
This PR is to add the framework for the
ThreeDomainSecureClient
component. Two additional methods ofisEligible
andshow
have been added to this component.Why are we making these changes? Include references to any related Jira tasks or GitHub Issues
https://paypal.atlassian.net/browse/DTPPCPSDK-2658
Reproduction Steps (if applicable)
Screenshots (if applicable)
Dependent Changes (if applicable)
Groups who should review (if applicable)
❤️ Thank you!