Skip to content

Fix: Enhance loadCoreSdkScript #845

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.

Sign up for GitHub

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

Closed
EvanReinstein wants to merge 7 commits into from
+205 −3
Closed

Fix: Enhance loadCoreSdkScript #845

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
dc861dd
add a condition and logic to resolve a pending script and prevent dou…
EvanReinstein Feb 27, 2026
fc8d478
chore: add changeset
EvanReinstein Feb 27, 2026
916d386
add tests
EvanReinstein Feb 27, 2026
e1cfe65
update to use custom window namespace
EvanReinstein Feb 27, 2026
47a755e
fix issues from auto-formatting
EvanReinstein Feb 27, 2026
6325d5c
update error message
EvanReinstein Feb 27, 2026
6093419
fix tests
EvanReinstein Feb 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/petite-banks-start.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@paypal/paypal-js": patch
---

Fixes an issue where loadCoreSdkScript would load 2 core scripts.
35 changes: 34 additions & 1 deletion packages/paypal-js/src/v6/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ function loadCoreSdkScript(options: LoadCoreSdkScriptOptions = {}) {
validateArguments(options);
const isServerEnv = isServer();

// Early resolve in SSR environments where DOM APIs are unavailable
if (isServerEnv) {
return Promise.resolve(null);
}
Expand All @@ -18,10 +19,41 @@ function loadCoreSdkScript(options: LoadCoreSdkScriptOptions = {}) {
'script[src*="/web-sdk/v6/core"]',
);

if (window.paypal?.version.startsWith("6") && currentScript) {
// Script already loaded and namespace is available — return immediately
if (window.paypal?.version?.startsWith("6") && currentScript) {
Copy link
Contributor

@gregjopa gregjopa Feb 27, 2026

Choose a reason for hiding this comment

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

Does this need to take into account the custom namespace option?

Suggested change
if (window.paypal?.version?.startsWith("6") && currentScript) {
const windowNamespace = options.dataNamespace ?? "paypal";
if (window[windowNamespace]?.version?.startsWith("6") && currentScript) {

Copy link
Contributor

@HackTheW2d HackTheW2d Feb 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Great catch! Same doubt here. But don't we always load at window.paypal internally? 🤔 Correct me if I am wrong :)

Copy link
Contributor

@gregjopa gregjopa Feb 27, 2026

Choose a reason for hiding this comment

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

In V6 we use window__paypal_sdk__ as the internal namespace. Here's an example when using data-namespace="paypalV6":

<script async="" src="https://www.sandbox.paypal.com/web-sdk/v6/core" data-namespace="paypalV6"></script>

Ex:

Image

Copy link
Contributor

@HackTheW2d HackTheW2d Feb 27, 2026

Choose a reason for hiding this comment

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

Thanks for the clarification! TIL

return Promise.resolve(window.paypal as unknown as PayPalV6Namespace);
Copy link
Contributor

@gregjopa gregjopa Feb 27, 2026

Choose a reason for hiding this comment

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

Same here with what gets returned.

Suggested change
return Promise.resolve(window.paypal as unknown as PayPalV6Namespace);
return Promise.resolve(window[windowNamespace] as unknown as PayPalV6Namespace);

Copy link
Contributor Author

@EvanReinstein EvanReinstein Feb 27, 2026

Choose a reason for hiding this comment

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

Screenshot 2026-02-27 at 3 14 10 PM

Confirmed that this update works with the custom namespace as well as window.paypal

}

// Script tag exists but hasn't finished loading yet (e.g., React StrictMode double-invoke)
if (currentScript) {
Copy link
Contributor

@HackTheW2d HackTheW2d Feb 27, 2026

Choose a reason for hiding this comment

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

Do we need to add some tests for this situation? since this impacts the load logic 🤔

Copy link
Contributor Author

@EvanReinstein EvanReinstein Feb 27, 2026

Choose a reason for hiding this comment

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

Yea great point, I will push some 👍

return new Promise<PayPalV6Namespace>((resolve, reject) => {
const namespace = options.dataNamespace ?? "paypal";
currentScript.addEventListener(
Copy link
Contributor

@gregjopa gregjopa Feb 27, 2026

Choose a reason for hiding this comment

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

If we go this route, we should look into sharing these callbacks with what is passed into insertScriptElement with onSuccess and onError. That function uses these same event listeners under the hood

Copy link
Contributor Author

@EvanReinstein EvanReinstein Feb 27, 2026

Choose a reason for hiding this comment

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

Ok i'll add this to my near-term ToDos

"load",
() => {
const paypalSDK = (
window as unknown as Record<string, unknown>
)[namespace] as PayPalV6Namespace | undefined;
if (paypalSDK) {
resolve(paypalSDK);
} else {
reject(
`The window.${namespace} global variable is not available`,
);
}
},
{ once: true },
);
currentScript.addEventListener(
"error",
() => {
reject(new Error(`The PayPal SDK script failed to load.`));
Copy link
Contributor

@HackTheW2d HackTheW2d Feb 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

This error message seems a bit vague to me. Can we make it more specific? :)

Suggested change
reject(new Error(`The PayPal SDK script failed to load.`));
reject(new Error(
`The script "${currentScript.src}" failed to load.
Check the HTTP status code and response body in DevTools to
learn more.`
));

},
{ once: true },
);
});
}

const { environment, debug, dataNamespace, dataSdkIntegrationSource } =
options;
const attributes: Record<string, string> = {};
Expand All @@ -44,6 +76,7 @@ function loadCoreSdkScript(options: LoadCoreSdkScriptOptions = {}) {
attributes["data-sdk-integration-source"] = dataSdkIntegrationSource;
}

// No existing script found — insert a new one and wait for it to load
return new Promise<PayPalV6Namespace>((resolve, reject) => {
insertScriptElement({
url: url.toString(),
Expand Down
Loading