adopted-ember-addons / ember-stripe-elements

A simple Ember wrapper for Stripe Elements
https://ember-stripe-elements.netlify.com
MIT License
19 stars 23 forks source link

Addon should retry upon script loading failure #22

Open jdurand opened 3 years ago

jdurand commented 3 years ago

I very often had issues in production where the Stripe script wouldn't load sending Error: Could not load script https://js.stripe.com/v3/to our Sentry logs. The route transition would then fail. My guess is network latency, DNS issues or maybe, yet unlikely, CDN issues with Stripe.

Before:

// route.js
beforeModel() {
  return this.get('stripe').load();
}

Le fix:

// route.js
beforeModel(transition) {
  const promise = this.get('stripe').load();
  promise.catch(() => {
    this.transitionTo('loading');
    run.later(() => {
      transition.retry();
    }, 2000);
  });
  return promise;
}

Since loading Stripe in the routes beforeModel hook is the recommended approach, I think the retry logic should be implemented in the addon and be a default. I'm thinking of something like this:

beforeModel() {
  return this.get('stripe').load({retryCount: 5, retryDelay: 1000}); // defaults: {retryCount: 3, retryDelay: 2000}
}

... perhaps with a non-linear (exponential?) delay after every retry.

I'd like the maintainers‘ opinion on this approach.

lindyhopchris commented 3 years ago

I don't see any problem with this, as long as it's opt in. I.e. by default just does the 1 try as per how it handles it at the moment.

The only problem with your proposed approach is the load method on the service already has two arguments: publishable key and stripe options. It would make sense to keep these addon options separate from the stripe options, so the two don't get confused.

So maybe something like this might be a solution:

return this.stripe.withRetry({ count: 5, delay: 1000 }).load();