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

[maxwelly-stripe]

Summary & motivation

When stripe-js fails to load, previously we would persist the error even on reload. Now, set stripePromise = null on error and if the load script is called again, we retrigger the load event to attempt to load stripe again.

Testing & documentation

I tested the repro stated here and verified that on reload we no longer see a persisting error and stripe gets reloaded properly.

[cbala-stripe]

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));?

[maxwelly-stripe]

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

[cbala-stripe]

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

[cbala-stripe]

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?

[maxwelly-stripe]

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!

[cbala-stripe]

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

[cbala-stripe]

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

[maxwelly-stripe]

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.

[fruchtose-stripe]

Thanks for working on this. I have a couple change requests.

[fruchtose-stripe]

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);
	});

[fruchtose-stripe]

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);

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[fxfilmxf]

@maxwelly-stripe @fruchtose-stripe is there still hope of potentially getting this change in? Does someone need to pick it up to get it over the finish line? I'd be more than willing if nobody else is working on this.

[maxwelly-stripe]

@fxfilmxf yes, i'll pick it back up next week, there was just some internal discussion on whether to additionally include automatic retries, but for now, we'll just address the caching issue and address automatic retries at a later date.

[fruchtose-stripe]

Almost there! Some additional changes are needed.

[fruchtose-stripe]

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?

[maxwelly-stripe]

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

[fruchtose-stripe]

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.

[fruchtose-stripe]

Incredible work!

[srakhmanchuk]

@maxwelly-stripe When is the planned release date for the new version that includes this fix?

[maxwelly-stripe]

@srakhmanchuk new release should be later today.

[maxwelly-stripe]

@srakhmanchuk New release should be up if you haven't seen it yet!

[fxfilmxf]

@maxwelly-stripe thank you!

[imasalygin]

@maxwelly-stripe Thank you. But I'm afraid only half of the problem was solved. The cache problem for the loadScript function was fixed. But, we use the loadStripe function, which uses the cached result of the only run of loadScript. So, loadStripe will always return a rejected promise if the first attempt fails. Do I understand correctly?

index.ts

const stripePromise = Promise.resolve().then(() => loadScript(null));

// will always return a rejected promise if the first and the only run of `loadScript` failed
export const loadStripe: LoadStripe = (...args) => {
  return stripePromise.then((maybeStripe) =>
    initStripe(maybeStripe, args, startTime)
  );
};

[maxwelly-stripe]

@imasalygin yes, you are correct that if you only run loadStripe once, you'll return the rejected promise and it won't automatically retry. That is on our radar of implementing automatic retries, however, that implementation needs more discussion before we follow through with it to iron out any potential issues with implementing automatic retries. For now, you can potentially implement a similar try catch to the repro in the issue you filed to retry loadStripe yourself in the case of the rejected promise. Hope that helps!

[imasalygin]

@maxwelly-stripe if the stripePromise variable contains a rejected promise, then the second call of loadStripe will be rejected too.
The problem is not about automatically retry, but about the stripePromise variable which always contains only the first attempt of loadScript

const stripePromise = Promise.resolve().then(() => {
  console.log('try to load');
  // return loadScript(null);
  return Promise.reject();
});

const loadStripe = () => {
  return stripePromise.then(() => {
    console.log('init');
  });
};

loadStripe();
loadStripe();
loadStripe();

'try to load' will be printed only once, it means that loadScript will called only once

[fxfilmxf]

@imasalygin The stripePromise variable is cleared when there's a rejection
https://github.com/stripe/stripe-js/pull/518/files#diff-3cc490302b79321eb38cc902ba2bc669600333b6e8428e4ac75981e3d5262550R146, so it should work as desired.

[imasalygin]

@fxfilmxf no, you show me code of loadScript function, I speak about the loadStripe function (loadStipe also has caching via stripePromise in the line 5)

https://github.com/stripe/stripe-js/blob/master/src/index.ts#L15

[fxfilmxf]

@imasalygin ah yes, you are correct. Apologies for the misunderstanding. The stripePromise in the loadStripe function is initialized once and never reset (which is what you said). And we aren't able to wrap loadStripe in a try-catch for the reasons you already stated.

@maxwelly-stripe what are your thoughts?

[maxwelly-stripe]

@fxfilmxf @imasalygin thanks for flagging this issue! i'll discuss more about this with my team and get back to you as soon as possible!