-
Notifications
You must be signed in to change notification settings - Fork 131
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.
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
Changes from all commits
dc861dd
fc8d478
916d386
e1cfe65
47a755e
6325d5c
6093419
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,16 +10,64 @@ 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); | ||
| } | ||
|
|
||
| const currentScript = document.querySelector<HTMLScriptElement>( | ||
| 'script[src*="/web-sdk/v6/core"]', | ||
| ); | ||
| const windowNamespace = options.dataNamespace ?? "paypal"; | ||
|
|
||
| if (window.paypal?.version.startsWith("6") && currentScript) { | ||
| return Promise.resolve(window.paypal as unknown as PayPalV6Namespace); | ||
| // Script already loaded and namespace is available — return immediately | ||
| if ( | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (window as Record<string, any>)[windowNamespace]?.version?.startsWith( | ||
| "6", | ||
| ) && | ||
| currentScript | ||
| ) { | ||
| return Promise.resolve( | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (window as Record<string, any>)[ | ||
| windowNamespace | ||
| ] as unknown as PayPalV6Namespace, | ||
| ); | ||
| } | ||
|
|
||
| // Script tag exists but hasn't finished loading yet (e.g., React StrictMode double-invoke) | ||
| if (currentScript) { | ||
| return new Promise<PayPalV6Namespace>((resolve, reject) => { | ||
| const namespace = options.dataNamespace ?? "paypal"; | ||
| currentScript.addEventListener( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 } = | ||
|
|
@@ -44,6 +92,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(), | ||
|
|
||
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 need to add some tests for this situation? since this impacts the load logic 🤔
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.
Yea great point, I will push some 👍