Skip to content
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

User reports network failure to load Stripe.js fails again after network is restored #518

Merged
merged 10 commits into from
Dec 8, 2023
+43 −13
Merged

User reports network failure to load Stripe.js fails again after network is restored #518

Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
30157ae
fix stripe promise reload
maxwelly-stripe Oct 23, 2023
072ac0a
remove whitespace
maxwelly-stripe Oct 23, 2023
e917010
yarn prettier
maxwelly-stripe Oct 23, 2023
157d06c
don't wrap error in new Error
maxwelly-stripe Oct 23, 2023
40a703e
pr feedback
maxwelly-stripe Dec 6, 2023
7e75d1e
prettier
maxwelly-stripe Dec 6, 2023
c24be62
remove event listeners
maxwelly-stripe Dec 6, 2023
ca49f7d
pr feedback
maxwelly-stripe Dec 6, 2023
23dd26a
add onloadlistener and onerror listener
maxwelly-stripe Dec 6, 2023
894a388
add onLoadListener and onErrorListener
maxwelly-stripe Dec 7, 2023
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
47 changes: 34 additions & 13 deletions src/shared.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,23 @@ const registerWrapper = (stripe: any, startTime: number): void => {

let stripePromise: Promise<StripeConstructor | null> | null = null;

const onError = (reject: (reason?: any) => void) => () => {
reject(new Error('Failed to load Stripe.js'));
};

const onLoad = (
resolve: (
value: StripeConstructor | PromiseLike<StripeConstructor | null> | null
) => void,
reject: (reason?: any) => void
) => () => {
if (window.Stripe) {
resolve(window.Stripe);
} else {
reject(new Error('Stripe.js not available'));
}
};

export const loadScript = (
params: null | LoadParams
): Promise<StripeConstructor | null> => {
Expand Down Expand Up @@ -96,26 +113,30 @@ export const loadScript = (
console.warn(EXISTING_SCRIPT_MESSAGE);
} else if (!script) {
script = injectScript(params);
} else if (script) {
// remove event listeners
script.removeEventListener('load', onLoad(resolve, reject));
script.removeEventListener('error', onError(reject));

// if script exists, but we are reloading due to an error,
// reload script to trigger 'load' event
script.parentNode?.removeChild(script);
fruchtose-stripe marked this conversation as resolved.
Show resolved Hide resolved
script = injectScript(params);
}

script.addEventListener('load', () => {
if (window.Stripe) {
resolve(window.Stripe);
} else {
reject(new Error('Stripe.js not available'));
}
});

script.addEventListener('error', () => {
reject(new Error('Failed to load Stripe.js'));
});
script.addEventListener('load', onLoad(resolve, reject));

script.addEventListener('error', onError(reject));
Copy link
Contributor

@fruchtose-stripe fruchtose-stripe Dec 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hmmmm, I forgot that resolve and reject need to be passed as arguments. You'll need to persist the listeners at the module level alongside the let stripePromise declaration. E.g.

let stripePromise = /* initializer */;
let onLoadListener; // <- Add type here
let onErrorListener; // <- Add type here

// loadScript
onLoadListener = onLoad(resolve, reject);
onErrorListener = onError(reject);
script.addEventListener('load', onLoadListener);
script.addEventListener('error', onErrorListener;

When calling removeEventListener(), you would then reference the onLoadListener and onErrorListener variables already defined. (And definitely check to make sure they're truthy.) Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay and just to clarify onLoadListener = onLoad(resolve, reject); the instances of onLoadListener would stay equivalent even when it's assigned again on retry?

Copy link
Contributor

Choose a reason for hiding this comment

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

The key here is only assigning onLoadListener and onErrorListener immediately before script.addEventListener(). If you assign "lazily," then they will be successfully removed on the next invocation in case of an error.

} catch (error) {
reject(error);
return;
}
});

return stripePromise;
// Resets stripePromise on error
return stripePromise.catch((error) => {
stripePromise = null;
return Promise.reject(error);
});
};

export const initStripe = (
Expand Down
Loading