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
Merged

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

Changes from 4 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
28 changes: 27 additions & 1 deletion src/shared.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,23 @@ const injectScript = (params: null | LoadParams): HTMLScriptElement => {
return script;
};

const reloadScript = (params: null | LoadParams): HTMLScriptElement => {
const queryString =
Copy link
Contributor

Choose a reason for hiding this comment

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

The query param logic should be shared with injectScript so it gets updated together.

params && !params.advancedFraudSignals ? '?advancedFraudSignals=false' : '';
const script = [...document.getElementsByTagName('script')].filter(
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it possible that we're altering script element that the merchant added themselves here? Is that what we want to be doing? Should we only affect scripts/DOM elements that we added ourselves?

Further, if this is the approach we want, why not use findScript to find the script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, I don't think we are since the filtering is specific to https://js.stripe.com/v3. But either way, I'm using @fruchtose-stripe's suggestion of using the existing script and removing it from it's parent node. Didn't think of that!

(element) => element.src === `${V3_URL}${queryString}`
)[0];
const headOrBody = document.head || document.body;
if (!headOrBody) {
throw new Error(
'Expected document.body not to be null. Stripe.js requires a <body> element.'
);
}
headOrBody.removeChild(script);

return injectScript(params);
};

const registerWrapper = (stripe: any, startTime: number): void => {
if (!stripe || !stripe._registerWrapper) {
return;
Expand Down Expand Up @@ -96,6 +113,10 @@ export const loadScript = (
console.warn(EXISTING_SCRIPT_MESSAGE);
} else if (!script) {
script = injectScript(params);
} else {
// if script exists, but we are reloading due to an error,
// reload script to trigger 'load' event
script = reloadScript(params);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is only retrying once right after the failure. Should we implement a backoff mechanism of some kind?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually not retrying at all unless the user re-calls loadStripe. We can plan to add automatic retries in the future, but for now, fixing the primary issue is the focus.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of querying the DOM for the script again, it would be simpler to remove it from the DOM and then reassign it. Here's my suggestion.

Suggested change
} else {
// if script exists, but we are reloading due to an error,
// reload script to trigger 'load' event
script = reloadScript(params);
} else {
// If `stripePromise` is unresolved but a script exists, there was an error loading it.
// We remove and reload the script anew.
script.parentNode.removeChild(script);
script = injectScript(params);

}

script.addEventListener('load', () => {
Expand All @@ -115,7 +136,12 @@ export const loadScript = (
}
});

return stripePromise;
// set stripePromise to null on error
return stripePromise
.catch((error) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this catch redundant? Can it be removed?

throw error;
})
.then((stripePromise = null));
Copy link
Contributor

Choose a reason for hiding this comment

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

This essentially removes the cache after each successful call. We should cache success and not cache failures. Did you intend to say .catch((stripePromise = null));?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm using @fruchtose-stripe's suggestion now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend unsetting the stripePromise variable in the catch block. Putting it in a then block could cause the variable to be unset for a resolved promise.

Suggested change
// set stripePromise to null on error
return stripePromise
.catch((error) => {
throw error;
})
.then((stripePromise = null));
// Resets stripePromise on error
return stripePromise
.catch((error) => {
stripePromise = null;
return Promise.reject(error);
});

};

export const initStripe = (
Expand Down
Loading