feat(v6): add support for deterministic loading#849
Conversation
🦋 Changeset detectedLatest commit: af06719 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 |
packages/paypal-js/src/v6/index.ts
Outdated
| SCRIPT_LOADING_STATE.RESOLVED, | ||
| ); | ||
| return resolve(paypalWindowReference); | ||
| }); |
There was a problem hiding this comment.
| }); | |
| }, { once: true }); |
to remove listener when triggered
There was a problem hiding this comment.
Great suggestion @HackTheW2d! I added this once option to both listeners
packages/paypal-js/src/v6/index.ts
Outdated
| SCRIPT_LOADING_STATE.REJECTED, | ||
| ); | ||
| return reject(defaultError); | ||
| }); |
There was a problem hiding this comment.
| }); | |
| }, { once: true }); |
same here
EvanReinstein
left a comment
EvanReinstein
left a comment
There was a problem hiding this comment.
This looks great @gregjopa Thanks for taking this up. I tested in the React sample integration and am only seeing 1 core script element and no console errors related to registering components more than once 👍
| expect(window.paypal).toBeDefined(); | ||
| }); | ||
|
|
||
| test("should avoid inserting two script elements when called twice", async () => { |
There was a problem hiding this comment.
currently we await the first load to be fully resolved then trigger the second one.
Do we want to add one more test to test concurrency like this? :) Thanks!
test("should handle concurrent calls without duplicating script elements",
async () => {
// no await here!
const promise1 = loadCoreSdkScript();
const promise2 = loadCoreSdkScript();
const [result1, result2] = await Promise.all([promise1, promise2]);
// and we only expect one insert
expect(scriptAppendChildSpy).toHaveBeenCalledTimes(1);
// both get resolved correctly without hanging
expect(result1).toBeDefined();
expect(result2).toBeDefined();
// they get same ref
expect(result1).toBe(result2);
// scriptElement to be resolved
const scriptElement = scriptAppendChildSpy.mock.calls[0][0];
expect(scriptElement.getAttribute("data-loading-state")).toBe("resolved");
});There was a problem hiding this comment.
I dig it! I added this parallel test 💯
HackTheW2d
left a comment
HackTheW2d
left a comment
There was a problem hiding this comment.
LGTM! Nice work! some non-blocking comments
3 participants
This PR adds a
data-loading-stateattribute to the V6 Core SDK script tag. This is leveraged to ensure that the v6 script only loads once, regardless of how many times theloadCoreSdkScript()function is called. This will help resolved any issue arounduseEffectrunning twice in react in strict mode.