Skip to content

Conversation

@EvanReinstein
Copy link
Contributor

@EvanReinstein EvanReinstein commented Sep 26, 2025

This PR adds a Provider wrapper that can be used to access SDK instance context. This is an initial set of basic changes that leverage paypal-js's loadCoreSdkScript and adds SSR related conditions to prevent/guard against running client-side code.

There is a corresponding PR here that implements the Provider and renders a PayPal and Venmo button. In order to test the Provider make sure to have cloned both repos and leverage npm link to create a symlink between the changes on this branch in react-paypal-js and the pre-built React one time payment example.

See the PR linked above for more specific step-by-step instructions.

* initial package.json and rollup updates, changes tsconfig to bundler and adds v6 specific tsconfig

* upgrade react-paypal-js typescript package to v5.3.3

* update tsconfig for v6 namespace

* chore: add changeset

* align rollup version and update playwright yml

* change playwright.yml download deps command

* update actions checkout and setup-node to v4

* update .nvmrc

* try different method for installing dependencies

* fix package dependency issues related to rollup and terser

* revert change to download deps step

* update package-lock

* test fix for rollup option deps bug

* update build script

* test adding cache to setup node action, update install command, and remove rollup force install in github workflows

* re-add force rollup install in github workflows

* run npm install to update package-lock with new package versions from typescript upgrade

* revert changes to download deps and setup node steps in github workflows

* install with update to package.json

* leverage fresh package-lock and remove all artifactory package resolution paths

* fix check annotations warnings

* add new line at the end of gitignore
@EvanReinstein EvanReinstein requested a review from a team as a code owner September 26, 2025 14:16
@changeset-bot
Copy link

changeset-bot bot commented Sep 26, 2025

🦋 Changeset detected

Latest commit: 1f7404c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@paypal/react-paypal-js Minor
@paypal/paypal-js Minor

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

scriptOptions: LoadCoreSdkScriptOptions;
}

export const PayPalInstanceProvider: React.FC<PayPalInstanceProviderProps> = ({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a fit of spontaneity I used "instance" as a key term in the names for the provider and related files, context state, etc.

I am not attached to this and am willing to make any changes. Originally I was calling this v6PayPalScriptProvider but felt that the provider was really providing the SDK instance. Any thoughts here would be appreciated.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this name makes sense, but what about adding sdk to make it clear what PayPal thing is being provided, i.e. PayPalSdkInstanceProvider?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that @kand I'll update and see what other folks have to say.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a v6-specific utils file. Only the folders/files within the src/v6/ path get built for V6 and importing utils from the V5 utils file was causing some issues. There is probably a workaround but this was the most straightforward solution.

Copy link
Contributor

@kand kand left a comment

Choose a reason for hiding this comment

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

Looks good!

scriptOptions: LoadCoreSdkScriptOptions;
}

export const PayPalInstanceProvider: React.FC<PayPalInstanceProviderProps> = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this name makes sense, but what about adding sdk to make it clear what PayPal thing is being provided, i.e. PayPalSdkInstanceProvider?

validateArguments(options);

// SSR safeguard
if (typeof document === "undefined") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me, but is this the standard way to check for SSR? I don't have much SSR experience so I'm curious.

Also, what's the difference between this check and isServer checking if typeof window === "undefined"? Would it make sense to use the same strategy in both places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shoot I meant to replace this with isServer.

I want to standardize on one isServer check, which currently is typeof window === "undefined". From looking at a few different packages (stripe, square, adyen-web) there are a few ways to do this, and typeof window === "undefined" is just one of those.

value: INSTANCE_LOADING_STATE.PENDING,
});
}
}, []); // Run once on mount
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Purposefully using an empty array here in order to run this effect only on mount.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like github / react is unhappy that state.loadingStatus is missing from the dependency array. I think we'll need it here because state is technically state that should cause this useEffect to update. What about putting a check at the top of the effect with a ref that checks if it's been run already?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, going to have to add a useRef to track this. At first I thought that since loadingStatus !== Initial after the first mount that a useRef wasn't necessary since the condition will never be fulfilled.

Massive but here...but it is safer and more explicit to leverage useRef. Let me push a change

state.scriptOptions,
]);

// Separate effect for eligibility - runs after instance is created
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently going with calling eligibility for all use cases. Originally we'd discussed passing the eligibility response to the provider and then conditionally calling eligibility. I'd like to update this portion of the provider when we implement the eligibility hook.

return newScript;
}

export const isServer = typeof window === "undefined";
Copy link
Contributor

Choose a reason for hiding this comment

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

Two minor things:

  1. should we make this a function so its callable in different contexts?
  2. Can we also check document? When doing server-side rendering, I think some environments may set window so its safer to check document. Might as well check for both being defined.
Suggested change
export const isServer = typeof window === "undefined";
export function isServer() {
return typeof window === "undefined" && typeof document === "undefined";
}

@EvanReinstein EvanReinstein force-pushed the feature/react-paypal-js-v6 branch from d41303e to a142270 Compare October 1, 2025 17:40
Copy link
Contributor

@kand kand left a comment

Choose a reason for hiding this comment

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

A couple small things, but looks great otherwise!

value: INSTANCE_LOADING_STATE.PENDING,
});
}
}, []); // Run once on mount
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like github / react is unhappy that state.loadingStatus is missing from the dependency array. I think we'll need it here because state is technically state that should cause this useEffect to update. What about putting a check at the top of the effect with a ref that checks if it's been run already?

Comment on lines 170 to 172
if (isServer()) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the note on line 101:

SDK loading effect - only runs on client (useEffect doesn't run during SSR)

Do you need this isServer check here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

Copy link
Contributor

@hamzanasir hamzanasir left a comment

Choose a reason for hiding this comment

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

Great job @EvanReinstein! Left a few comments

* {children}
* </PayPalSdkInstanceProvider>
*/
export const PayPalSdkInstanceProvider: React.FC<
Copy link
Contributor

Choose a reason for hiding this comment

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

I would actually vote name this more generic. Something like <PayPalProvider /> or <PayPalCheckoutProvider />. I think that the word SdkInstance doesn't hold a lot of meaning for the merchant since the only consumers of this provider will be our hooks/components. Stripe has something similar I believe:

<CheckoutProvider stripe={stripePromise} options={{clientSecret: promise}}>
      <CheckoutForm />
</CheckoutProvider>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Point taken!

value: INSTANCE_LOADING_STATE.PENDING,
});
}
}, []); // Run once on mount
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

*
* @example
* <PayPalSdkInstanceProvider
* createInstanceOptions={{
Copy link
Contributor

Choose a reason for hiding this comment

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

Once again I don't think createInstanceOptions will ring a bell to anyone who hasn't used the v6 SDK before. I would vote for a more abstracted flattened provider like this one:

<PayPalCheckoutProvider
    components: ["paypal-payments"],
    clientToken: token,
    environment: "sandbox"
>
    {children}
</PayPalSdkInstanceProvider>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I considered this and tbh it would be helpful in terms of inline objects passed as props causing rerenders, which the provider then needs to manage.

If other folks don't have strong opinions against this I'll go down this path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hamzanasir Alrighty, I updated the naming convention across the board. Lmk what you think

Copy link
Contributor

@hamzanasir hamzanasir left a comment

Choose a reason for hiding this comment

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

Great work @EvanReinstein!

url: url.toString(),
onSuccess: () => {
if (!window.paypal) {
if (isServerEnv || !window.paypal) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we even get to this line of code if isServerEnv is true? I would assume the above SSR safeguard does an early return

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh shoot, correct. Let me push that change

* You can pass options directly without manual memoization.
*
* @example
* <PayPalSdkInstanceProvider
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* <PayPalSdkInstanceProvider
* <PayPalProvider

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch!

Comment on lines 116 to 119
memoizedCreateOptions,
memoizedScriptOptions,
state.createInstanceOptions,
state.scriptOptions,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused by this logic. Why are we listening to both options for changes?

return;
}

let isSubscribed = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain what this does exactly? To my knowledge once the hook calls the cleanup function this useEffect will be lost so it shouldn't run anymore. Do we need this isSubscribed logic?

Copy link
Contributor

@imbrian imbrian Oct 6, 2025

Choose a reason for hiding this comment

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

It's an attempt to prevent loadSdk from continuing to run with an update happening in the background. It's async, and its steps are also async. It probably would benefit from a check before await loadCoreSdkScript though, since we're already in deferred operation at that point.

I think if you wanted to clean it up, you could use an AbortController instead. Haven't looked at what APIs are used in loadCoreSdkScript to see if any of them actually take an AbortSignal, but you could write your own, if this is going to be a common pattern.


// SSR safeguard
if (isServerEnv) {
return Promise.resolve(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want a noop here or a warning? Presumably this would only be called in a useEffect, which won't be run until the client. Seems like this being called on the server is probably more indicative of an integration problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point on adding a warning!

return;
}

let isSubscribed = true;
Copy link
Contributor

@imbrian imbrian Oct 6, 2025

Choose a reason for hiding this comment

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

It's an attempt to prevent loadSdk from continuing to run with an update happening in the background. It's async, and its steps are also async. It probably would benefit from a check before await loadCoreSdkScript though, since we're already in deferred operation at that point.

I think if you wanted to clean it up, you could use an AbortController instead. Haven't looked at what APIs are used in loadCoreSdkScript to see if any of them actually take an AbortSignal, but you could write your own, if this is going to be a common pattern.

const loadEligibility = async () => {
try {
const eligiblePaymentMethods =
await state.sdkInstance?.findEligibleMethods({});
Copy link
Contributor

Choose a reason for hiding this comment

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

Does there need to be a mechanism for the merchant to override the necessity of this call? Can the merchant provide their own server-side eligibility determination?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that work is forthcoming 👍 and this is more of a placeholder.

@EvanReinstein EvanReinstein merged commit dda72d7 into feature/react-paypal-js-v6 Oct 7, 2025
2 checks passed
@EvanReinstein EvanReinstein deleted the feature/DTPPCPSDK-3479-provider branch October 7, 2025 14:27
EvanReinstein added a commit that referenced this pull request Nov 6, 2025
* Feat: Upgrade Typescript to v5 in react-paypal-js (#685)

* initial package.json and rollup updates, changes tsconfig to bundler and adds v6 specific tsconfig

* upgrade react-paypal-js typescript package to v5.3.3

* update tsconfig for v6 namespace

* chore: add changeset

* align rollup version and update playwright yml

* change playwright.yml download deps command

* update actions checkout and setup-node to v4

* update .nvmrc

* try different method for installing dependencies

* fix package dependency issues related to rollup and terser

* revert change to download deps step

* update package-lock

* test fix for rollup option deps bug

* update build script

* test adding cache to setup node action, update install command, and remove rollup force install in github workflows

* re-add force rollup install in github workflows

* run npm install to update package-lock with new package versions from typescript upgrade

* revert changes to download deps and setup node steps in github workflows

* install with update to package.json

* leverage fresh package-lock and remove all artifactory package resolution paths

* fix check annotations warnings

* add new line at the end of gitignore

* Feat: V6 Context Provider (#688)

* Feat: Upgrade Typescript to v5 in react-paypal-js (#685)

* initial package.json and rollup updates, changes tsconfig to bundler and adds v6 specific tsconfig

* upgrade react-paypal-js typescript package to v5.3.3

* update tsconfig for v6 namespace

* chore: add changeset

* align rollup version and update playwright yml

* change playwright.yml download deps command

* update actions checkout and setup-node to v4

* update .nvmrc

* try different method for installing dependencies

* fix package dependency issues related to rollup and terser

* revert change to download deps step

* update package-lock

* test fix for rollup option deps bug

* update build script

* test adding cache to setup node action, update install command, and remove rollup force install in github workflows

* re-add force rollup install in github workflows

* run npm install to update package-lock with new package versions from typescript upgrade

* revert changes to download deps and setup node steps in github workflows

* install with update to package.json

* leverage fresh package-lock and remove all artifactory package resolution paths

* fix check annotations warnings

* add new line at the end of gitignore

* add ssr checks to load core sdk script func and add initial instance provider

* add a basic set of hooks

* update exports

* mark usePayPalInstance hook as exportable

* add isServer util and apply where appropriate

* clean up usePayPalInstance file

* update exports

* create v6 specific utils file

* add reducer to manage various state, fix missed isServer condition in paypal-js

* update paypal sdk instance provider name

* remove json.stringify operation from memoizedOptions

* clean up useMemo

* clean up useMemo import

* update provider to account for useEffect during ssr

* add tests for client-side and server-side instance provider

* add instance provider context test

* add context test and clean up other provider tests

* update initial to pending state useEffect with empty dependency array

* configure provider to handle memoization of create instance and script options

* memoize context to prevent rerenders

* remove duplicate isServer

* swap deep equality check packages

* update isServer to a function that also checks whether the document is present

* fix tests and missed isServer uses

* remove isServer from eligibility useEffect

* add changeset

* add ref for initial hydration

* remove abort controller

* flatten create instance options props object

* update naming convention

* update tests to reflect name and prop passing changes

* remove dead code isServer check

* update jsdoc

* update imports to correct values

* add console warning to loadCoreSdkScript

* remove create instance and script options from reducer state

* remove sdkInstance check in loading useEffect

* Feat: Add check for existing core script (#701)

* update loadCoreSdkScript with a condition to check if a core script already exists

* enhance window.paypal check to include version

* chore: add changeset

* feat: Implement use venmo one time payment session hook

* moved `useProxyProps` to this branch

* rename to types file

* CreateOrderPromise

* export CreateOrder* types

* chore: Breakout enums from type file

* add pay later hook

* fix venmo test file

* rename test file

* fix types

* fix tests and types

* add typecheck to lint command for CI

* include all files for tsconfig and fix type errors

* Feat: usePayPalOneTimePaymentSession hook (#699)

* add inital paypal otp payment session hook

* add tests

* address review comments

* update hook after boilerplate agreement

* remove hook jsdoc comment

* address review comments

* review pr comments

* Refactor tests to mock useProxyProps

* add pay later hook

* refactor after convo

* some notes

* remove unused code

* wip

* rename types file

* use correct types

* useProxyProps

* fix error message

* remove comment

* tests placeholder

* add test watch command

* test stubs

* wip

* wip

* tests wip

* use optional chaining

* proxy callback test

* some docs

* test wip

* test wip

* fixed

* cleanup

* rename

* some cleanup

* fix venmo test file

* add tests

* update tests

* remove callback proxying tests

---------

Co-authored-by: Hamza Nasir <munasir@paypal.com>
Co-authored-by: Muhammad Hamza Nasir <mhamzanasir97@gmail.com>
Co-authored-by: Andrew Kos <akos123@gmail.com>

* Feat: usePayPalSavePaymentSession hook  (#708)

* add paypal save payment session hook

* fix typo

* fix another typo

* Refactor PayPalProvider (#714)

* initial provider refactoring and deep compare updates

* fix infinite loop and refactor create sdk instance useEffect

* remove ts doc comment for the provider

* refactor provider tests

* remove dequal from react-paypal-js dependencies

* remove initial from instance loading state enum

* add jsdoc for isEqualComponentsArray helper

* fix test

* revert paypal-js changes

* remove shared test utils file (#723)

* remove shared test utils file

* update load core sdk script tests

* add a changeset

* Revert "update load core sdk script tests"

This reverts commit 8e8e712.

* re-apply test fix

* Feat: Add dataNamespace to the PayPal Provider (#720)

* add datanamespace option to the provider

* update exported session hooks

* Fix/refactor types (#725)

* refactor pay later types

* refactor paypal otp types

* remove provider type declaration file

* remove venmo type declaration file

* remove save payment declaration file and add base payment session return interface

* apple base payment session return type

* fix build errors

* v6 React error handling (#721)

* error handling

* update state type, remove dispatch

* update package-lock

* fix types

* fix some warnings

* remove sdk instance from expect rejected state

* remove unused code

* skip eligibility error for now

* fix error

* fix changes from merge conflicts

---------

Co-authored-by: Hamza Nasir <munasir@paypal.com>
Co-authored-by: Andrew Kos <akos123@gmail.com>
Co-authored-by: Muhammad Hamza Nasir <mhamzanasir97@gmail.com>
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.

6 participants