Conversation
…ble loading core scripts
🦋 Changeset detectedLatest commit: 6093419 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 |
EvanReinstein
changed the title
| } | ||
|
|
||
| // Script tag exists but hasn't finished loading yet (e.g., React StrictMode double-invoke) | ||
| if (currentScript) { |
There was a problem hiding this comment.
Do we need to add some tests for this situation? since this impacts the load logic 🤔
There was a problem hiding this comment.
Yea great point, I will push some 👍
packages/paypal-js/src/v6/index.ts
Outdated
|
|
||
| if (window.paypal?.version.startsWith("6") && currentScript) { | ||
| // Script already loaded and namespace is available — return immediately | ||
| if (window.paypal?.version?.startsWith("6") && currentScript) { |
There was a problem hiding this comment.
Does this need to take into account the custom namespace option?
| if (window.paypal?.version?.startsWith("6") && currentScript) { | |
| const windowNamespace = options.dataNamespace ?? "paypal"; | |
| if (window[windowNamespace]?.version?.startsWith("6") && currentScript) { |
There was a problem hiding this comment.
Great catch! Same doubt here. But don't we always load at window.paypal internally? 🤔 Correct me if I am wrong :)
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
Thanks for the clarification! TIL
packages/paypal-js/src/v6/index.ts
Outdated
| if (window.paypal?.version.startsWith("6") && currentScript) { | ||
| // Script already loaded and namespace is available — return immediately | ||
| if (window.paypal?.version?.startsWith("6") && currentScript) { | ||
| return Promise.resolve(window.paypal as unknown as PayPalV6Namespace); |
There was a problem hiding this comment.
Same here with what gets returned.
| return Promise.resolve(window.paypal as unknown as PayPalV6Namespace); | |
| return Promise.resolve(window[windowNamespace] as unknown as PayPalV6Namespace); |
There was a problem hiding this comment.
Confirmed that this update works with the custom namespace as well as window.paypal
packages/paypal-js/src/v6/index.ts
Outdated
| currentScript.addEventListener( | ||
| "error", | ||
| () => { | ||
| reject(new Error(`The PayPal SDK script failed to load.`)); |
There was a problem hiding this comment.
This error message seems a bit vague to me. Can we make it more specific? :)
| 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.` | |
| )); |
| if (currentScript) { | ||
| return new Promise<PayPalV6Namespace>((resolve, reject) => { | ||
| const namespace = options.dataNamespace ?? "paypal"; | ||
| currentScript.addEventListener( |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Ok i'll add this to my near-term ToDos
4 participants
loadCoreSdkScripthas a condition that prevents duplicate script loads, however there is an edge-case in the current logic. If loadCoreSdkScript is called twice (like by React StrictMode for example) the core script is added to the DOM and is in the process of loading, i.e.window.paypalis still undefined, when the function runs for the second time. The current condition evaluates to false, resulting in another script added to the DOM.This change updates
loadCoreSdkScriptto prevent the double load.