CardFieldsProvider and context created #754
Conversation
🦋 Changeset detectedLatest commit: bdc987d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
AleGastelum
changed the title
| // Early return: Still loading, wait for sdkInstance | ||
| if (loadingStatus === INSTANCE_LOADING_STATE.PENDING) { | ||
| return; | ||
| } | ||
|
|
||
| // Error case: Loading finished but no sdkInstance | ||
| if (!sdkInstance) { | ||
| const errorMsg = toError("no sdk instance available"); | ||
| setError(errorMsg); | ||
| setCardFieldsError(errorMsg); | ||
| return; | ||
| } |
There was a problem hiding this comment.
I'm wondering if we want to handle the error case in a separate useEffect? The hooks follow this pattern.
The early return AND the error handling together effectively do the same thing as the hook's strategy, this is merely a suggestion.
There was a problem hiding this comment.
I think it might be good to follow the pattern other hooks/components are following. Thanks for mentioning it, I will make the update
There was a problem hiding this comment.
Took another look at this—the separate useEffect pattern in this file seems to be addressing an infinite looping issue we don't have here. I'm leaning towards keeping it in one useEffect for simplicity and maintainability, but wanted to get your thoughts. Is there another benefit to splitting it that I'm not seeing?
There was a problem hiding this comment.
I think you're correct in the difference between this use case and the hooks'. The only benefit is having a separate effect that will handle errors unrelated to creating the session, but as you said it may impact simplicity and maintainability.
…ollow stablished pattern
| export { PayPalOneTimePaymentButton } from "./components/PayPalOneTimePaymentButton"; | ||
| export { PayPalProvider } from "./components/PayPalProvider"; | ||
| export { usePayPal } from "./hooks/usePayPal"; | ||
| export { CardFieldsProvider } from "./components/CardFieldsProvider"; |
There was a problem hiding this comment.
Any specific reason why useCardFields hook is not exported? Currently we are exporting all our hooks. Is there any different intended usage pattern that we want to access CardFields session?
There was a problem hiding this comment.
Good question. useCardFields hook will be internally used by other Card Field components and hooks (that will be added in following PRs) to manage session usage. Those components will then make sure to export anything the merchant needs from the session. That is why only the provider is needed by the merchant and not the hook.
EvanReinstein
left a comment
EvanReinstein
left a comment
There was a problem hiding this comment.
Lgtm! A couple of small comments but otherwise, glad to see Card Fields cruising along 👍
| return { html, cardFieldsSessionState, cardFieldsStatusState }; | ||
| } | ||
|
|
||
| function setupSSRTestComponent() { |
There was a problem hiding this comment.
What makes this "SSR" specifically?
|
|
||
| jest.mock("../utils", () => ({ | ||
| ...jest.requireActual("../utils"), | ||
| isServer: () => true, |
There was a problem hiding this comment.
Also, I don't see the provider using isServer anywhere, should it be?
| test("should verify isServer mock is working", () => { | ||
| expect(isServer()).toBe(true); | ||
| }); |
There was a problem hiding this comment.
This is a strange test, did this come from AI? I don't think this test suite should be checking this.
|
|
||
| test("should not attempt DOM access during server rendering", () => { | ||
| // In Node environment, document should be undefined | ||
| expect(typeof document).toBe("undefined"); |
There was a problem hiding this comment.
IMO, if we're meaning to test SSR, we should use and mock isServer for these tests rather than checking if the document is undefined directly.
There was a problem hiding this comment.
These tests are weird to me since we don't have SSR logic in the provider. Some of these tests seem incorrect, others are testing functionality I'm not sure we should be testing for SSR specifically (e.g. Context isolation), and some I don't think we're actually specifically coding for (e.g. should maintain state consistency across multiple server renders).
IMO we should add these when we have SSR logic, and we should be more specific / careful about what we're testing.
There was a problem hiding this comment.
Great callout! This tests were implemented following the tests PayPalProvider had for SSR. This subject was brought up with the team. The SSR tests files for Card fields and PayPal providers will be removed in a new PR. SSR tests will be added as needed.
Adds the following:
The Card Fields Provider creates, manages, and shares session with child components that will need access to it.