Skip to content
This repository has been archived by the owner on Mar 27, 2021. It is now read-only.

Stripe async example and README doesn't always load properly #154

Closed
ljbade opened this issue Jan 18, 2018 · 21 comments
Closed

Stripe async example and README doesn't always load properly #154

ljbade opened this issue Jan 18, 2018 · 21 comments

Comments

@ljbade
Copy link

ljbade commented Jan 18, 2018

We implemented the async Stripe loading following the README and example. However users found that the Stripe form did not always load properly and required a few page refreshes for it to load.

We discovered an improvement that fixes this issue by checking if the script had already finished loading before the component was created. For example if the Stripe form is on another container from the home page.

The change we made was:

  componentDidMount() {
    if (window.Stripe) {
      this.state = { stripe: window.Stripe(this.props.stripeConfig.key) };
    } else {
      document.querySelector('#stripe-js').addEventListener('load', () => {
        this.setState({ stripe: window.Stripe(this.props.stripeConfig.key) });
      });
    }
  }

/cc @tonyd256

@ljbade
Copy link
Author

ljbade commented Jan 18, 2018

I added a check that window exists to ensure server side rendering doesn't crash.

@jez
Copy link
Collaborator

jez commented Jan 18, 2018

Hey @ljbade, thanks for reaching out!

But I'm a little confused. Are you suggesting that this change should be made somewhere in our code? It looks rather similar to this example code from the README:

class App extends React.Component {
  constructor() {
    this.state = {stripe: null};
  }
  componentDidMount() {
    document.querySelector('#stripe-js').addEventListener('load', () => {
      // Create Stripe instance once Stripe.js loads
      this.setState({stripe: window.Stripe('pk_test_12345')});
    });
  }
  render() {
    // this.state.stripe will either be null or a Stripe instance
    // depending on whether Stripe.js has loaded.
    return (
      <StripeProvider stripe={this.state.stripe}>
        <Elements>
          <InjectedCheckoutForm />
        </Elements>
      </StripeProvider>
    );
  }
}

The difference is that instead of running in the constructor, we run in componentDidMount so that we know window is defined (componentDidMount only runs in the browser, so it's server side friendly to use window.Stripe directly there).

It might help me to understand if you could put together a JSFiddle that reproduces the issue you mention running into.

@tonyd256
Copy link

@jez The problem is that Stripe could already be loaded by the time componentDidMount is called. That means that the load event will never be captured. We could check for the existence of window.Stripe in componentDidMount and set the state then but I think it's not good practice to set the state in that function.

@jez
Copy link
Collaborator

jez commented Jan 19, 2018

Ah, okay. Good catch!

In that case, would this change work?

diff --git a/app.js b/app.js
index e5ebb35..110de30 100644
--- a/app.js
+++ b/app.js
@@ -3,11 +3,15 @@ class App extends React.Component {
     this.state = {stripe: null};
   }
   componentDidMount() {
+    if (window.Stripe) {
+      this.setState({stripe: window.Stripe('pk_test_12345')});
+    } else {
       document.querySelector('#stripe-js').addEventListener('load', () => {
         // Create Stripe instance once Stripe.js loads
         this.setState({stripe: window.Stripe('pk_test_12345')});
       });
     }
+  }
   render() {
     // this.state.stripe will either be null or a Stripe instance
     // depending on whether Stripe.js has loaded.

This is the relevant section of the docs for using setState in componentDidMount:

Calling setState() in this method will trigger an extra rendering, but it will happen before the browser updates the screen. This guarantees that even though the render() will be called twice in this case, the user won’t see the intermediate state. Use this pattern with caution because it often causes performance issues. It can, however, be necessary for cases like modals and tooltips when you need to measure a DOM node before rendering something that depends on its size or position.

If you really want to avoid drawing out the first render, you can do the check in a
setTimeout(..., 0) call to force the window.Stripe check to the next tick.

@ljbade
Copy link
Author

ljbade commented Jan 19, 2018

I updated the original comment with the final code we have that works properly with SSR.

@ljbade
Copy link
Author

ljbade commented Jan 19, 2018

@jez perfect thats the same code we ended up going with.

@tonyd256
Copy link

It's actually a little different. It's an anti-pattern to call setState in componentDidMount. We're using this.state.stripe = instead but I still don't think this is good practice in React.

@jez
Copy link
Collaborator

jez commented Jan 19, 2018

I think that actually the best solution is to use dynamic script tag injection, like this:

https://github.com/stripe/react-stripe-elements/blob/7bc56c589b6348b7255937a62aafed163934f078/demo/async/async.js#L99-L113

But I've opened up a PR which makes the change to componentDidMount. I think that there are tradeoffs to all the listed solutions, so to some extent all solutions will involve an arbitrary judgement call.

@mmeo
Copy link

mmeo commented May 21, 2018

Ran into this issue with Stripe.js too. Worked most of the time, minus when the mount function wouldn't fire.

I did exactly what Jez recommended above re:

setTimeout(function(){
// Stripe stuff
}, 500);

Solved it perfectly.

@mrcoles
Copy link

mrcoles commented Feb 12, 2019

here’s a single Provider component I just wrote to load the stripe.js script asynchronously, which I based on that demo code: AsyncStripeProvider.js

@rhys-vdw
Copy link

rhys-vdw commented Jul 24, 2019

Does anyone have advice on how to handle the situation in which Stripe fails to download (for whatever reason). Before stripe-elements I used this (admittedly complicated) technique to force all operations that used Stripe to either wait for the download to complete or handle the error.

It's unclear to me how I can do this using the StripeProvider which will just provide undefined without clarifying the reason it's undefined—either not completed downloading yet, or an error was encountered.

I was hoping that stripe elements would handle all this complicated stuff for me, but after switching to the simplified version we're still hitting this error in production:

Error: Please load Stripe.js (https://js.stripe.com/v3/) on this page to use react-stripe-elements[....]

@wichopy
Copy link

wichopy commented Jul 24, 2019

I wrapped StripeProvider in my own components and do the dynamic script loading inside of componentDidMount. This allowed me to hook my react callback functions up to the onload and onerror callbacks. When onload is called, I initialize the Stripe library myself and pass the instance to the StripeProvider.

  // componentDidMount
   if (!window.Stripe) {
      const stripeJs = document.createElement('script');
      stripeJs.src = 'https://js.stripe.com/v3/';
      stripeJs.async = true;
      stripeJs.onload = () => {
        this.props.onLoad();
        this.setState({
          stripe: window.Stripe(this.props.stripePublicKey),
        });
      };

      stripeJs.onerror = (err) => {
        this.props.onError();
        this.setState({
          error: 'Something went wrong trying to load our credit card form.  Please try again later',
        });
      };

      document.body.appendChild(stripeJs);

   }
  

// render
   return <StripeProvider stripe={this.state.stripe}>
      <Elements>
        {this.props.children}
      </Elements>
    </StripeProvider>;

I also wrapped the CardElement in my own component as we got errors with people pressing submit buttons before stripe loaded. There is an onReady prop that I hooked onto so that I could disable any submission buttons until the onReady got fired.

/// render
        <CardElement
          onReady={this.props.onReady}
        />

@rhys-vdw
Copy link

@wichopy ah, interesting. I've put my StripeProvider on the root mount so that it's available to the whole app (I believe it's advised to load stripe on every page load for fraud-detection reasons). So I guess here i'd have to wrap the StripeProvider in my own custom provider that provides both the error (or null) and the stripe instance (or null).

@wichopy
Copy link

wichopy commented Jul 24, 2019

@rhys-vdw Yep you can set it up so that whenever you have a CardForm you wrap it in the custom provider. I did not put it in the root mount to save a bit on initial load times on pages that don't need a credit card form. Its a bit more work putting the Provider in multiple pages but it has been working out for us so far.

@javidjamae
Copy link

javidjamae commented Aug 14, 2019

FWIW - I'm using injectStripe in my payment form (a modal) and I simply defer loading the library by putting the following in the component that wraps it (no need to put it at the App level as suggested in the demo / docs):

  componentWillMount() {
    if ( !window.Stripe ) {
      const stripeJs = document.createElement( 'script' )
      stripeJs.src = 'https://js.stripe.com/v3/'
      stripeJs.async = true
      document.body && document.body.appendChild( stripeJs )
    }
  }

I didn't need to mess with state or handle any callback.

The user has to take a couple actions before our payment modal appears, so I don't have any issues with the library not being loaded by the time the user needs it.

@pablogarcor
Copy link

here’s a single Provider component I just wrote to load the stripe.js script asynchronously, which I based on that demo code: AsyncStripeProvider.js

This is the only thing that have helped me. I am using state hooks in a simple component, I did almost everything and nothing worked but this.

@daviseford
Copy link

here’s a single Provider component I just wrote to load the stripe.js script asynchronously, which I based on that demo code: AsyncStripeProvider.js

Hey guys, I loved this, but I like using Hooks even more. Here's my version of this component using hooks: https://gist.github.com/daviseford/4a0ed622dc47090fe22c1870217d88d6

My version also protects against adding the stripe script more than once to the document!

@zehawki
Copy link

zehawki commented Oct 25, 2019

@jez

Perhaps this bit document.querySelector('#stripe-js') needs to be changed to document.(querySelector('script[src="https://js.stripe.com/v3/"]'). I'm guessing due to script changes, this has gone a bit out of sync?

oh, code also needs a removeEventListener...

@tonyd256

It's an anti-pattern to call setState in componentDidMount.

Incorrect

We're using this.state.stripe =

Also incorrect :-). Dont do direct state mutation.

@r3wt
Copy link

r3wt commented Dec 2, 2019

all of the shown solutions will continually create new instances of stripe on the window, resulting iframes stacking up in the browser. In our app, we only want the stripe instance to mount for certain routes, so we wrapped all of our Billing routes in a catchall route that mounts the stripe provider and then mounts our billing routes like so:

<Route path='/billing' render={()=>(
    <AsyncStripeProvider apiKey={config.stripeKey}>
        <Elements>
            <Route path='/billing/payment-method' exact component={PaymentMethod} />
        </Elements>    
    </AsyncStripeProvider>
)} />

We quickly discover that everytime we enter a billing page, a new Stripe Iframe is added, because the AsyncStripeProvider will create a new instance everytime it is mount. This is the workaround we came with ( fork of code by @mrcoles ):

import React, { Component } from 'react';
import { StripeProvider } from 'react-stripe-elements';

export default class AsyncStripeProvider extends Component {

    state = {
        stripe: null
    }

    // life-cycle
    componentDidMount() {

        this._mounted = true;

        const stripeJsElement = document.getElementById('my-stripe-js');

        if(!stripeJsElement) {
            const stripeJs = document.createElement('script');
            stripeJs.id = 'my-stripe-js';
            stripeJs.src = 'https://js.stripe.com/v3/';
            stripeJs.async = true;
            stripeJs.onload = () => {
                if (this._mounted) {
                    if(window._$stripe_instance===undefined){
                        window._$stripe_instance = window.Stripe(this.props.apiKey)
                    }
                    this.setState({
                        stripe: window._$stripe_instance
                    });
                }
            };
            document.body && document.body.appendChild(stripeJs);
        }else{
            this.setState({
                stripe: window._$stripe_instance
            });
        }

    }

    componentWillUnmount() {
        this._mounted = false;
    }

    // render
    render() {

        const { stripe } = this.state;

        return (
            <StripeProvider stripe={stripe}>
                {this.props.children}
            </StripeProvider>
        );
    }
}

if anyone knows a better solution than slapping the instance on window, please advise

@wichopy
Copy link

wichopy commented Dec 2, 2019

@r3wt I am loading a few external scripts in my project and noticed I was doing the same thing everytime with them.

My approach was making a generic script loader class that would keep track of which scripts loaded, instead of storing this state in the window object. It also stores an inflightPromises Map so if you have other components that are waiting for the same script, they can know when its done loading and do thier onSuccess work.

class ScriptLoader {
  constructor() {
    this.loadedSDKs = [];
    this.inflightRequests = new Map();
    this.sdks = {
      stripe: {
        src: 'https://js.stripe.com/v3/',
        id: 'stripe-jssdk',
        globalName: 'Stripe',
      },
      twitter: {
        src: 'https://platform.twitter.com/widgets.js',
        id: 'twitter-wjs',
        globalName: 'twttr',
      },
      // ...etc
    }

    this.load = (name) => {
      if (this.loadedSDKs.includes(name)) {
        return Promise.resolve();
      }

      const inFlight = this.inflightRequests.get(name);
      if (inFlight) {
        return inFlight;
      }

      const inflightPromise = new Promise((res, rej) => {
        // Note: This will break if your HTML does not have at least 1 script tag present.
        const firstJSTag = document.getElementsByTagName('script')[0];
        const sdk = document.createElement('script');
        sdk.src = this.sdks[name].src;
        sdk.async = true;
        sdk.id = this.sdks[name].id;
        sdk.onerror = (err) => {
          rej(err);
        };
        sdk.onload = () => {
          res();
          this.loadedSDKs.push(name);
          this.inflightRequests.delete(name);
        };
        firstJSTag.parentNode.insertBefore(sdk, firstJSTag);
      });

      this.inflightRequests.set(
        name,
        inflightPromise,
      );

      return inflightPromise;
    };

    this.isLoaded = (name) => {
      return this.loadedSDKs.includes(name);
    };
  }

}


const scriptLoader = new ScriptLoader();

// Singleton, we only want one instance of SDK loader per app.
export default scriptLoader;

Where ever you load your Stripe Provider you can use the ScriptLoader class like so to hook into React.

import { StripeProvider, Elements } from 'react-stripe-elements';
import ScriptLoader from './scriptloader';

// ...
componentDidMount() {
  if (ScriptLoader.isLoaded('stripe')) {
    this.props.onLoad();
    this.setState({
      stripe: window.Stripe(this.props.stripePublicKey),
    });
  } else {
    ScriptLoader.load('stripe')
      .then(() => {
        this.props.onLoad();
        this.setState({
          stripe: window.Stripe(this.props.stripePublicKey),
        });
      })
      .catch((err) => {
        this.props.onError();
      });
  }
}

// ...

return <StripeProvider stripe={this.state.stripe}>
  <Elements>
    {this.props.children}
  </Elements>
</StripeProvider>;

@mrcoles
Copy link

mrcoles commented Dec 3, 2019

@r3wt good point! I think I have mine around my root app, since Stripe claims it helps them better identify fraud (however I encounter your problem with HMR reloads in dev). Can you not just check if window.Stripe === undefined and have your code only inject the script if it isn’t. That would save you the extra step of creating another global object on window. It’s not perfect (e.g., if two different components load quickly in succession causing it to inject another one before the first one has loaded), but a pretty simple way to check and no worse than your example. (Or am I missing something—does calling window.Stripe(API_KEY) create the iframe?)

@wichopy cool script loader! Why not simplify your componentDidMount code to just use the promise regardless? Also, for completeness, if we want to avoid “Warning: Can't call setState on an unmounted component.” errors, we can add a check for that too:

componentDidMount() {
  this._mounted = true;
  ScriptLoader.load('stripe')
    .then(() => {
      if (this._mounted) {
        this.props.onLoad();
        this.setState({
            stripe: window.Stripe(this.props.stripePublicKey),
        });
      }
    })
    .catch((err) => {
      this.props.onError();
    });
  }
}

componentWillUnmount() {
  this._mounted = false;
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests