Improve type checking for card verify #2057

Merged
amelzer merged 1 commit from types/cardverify into master 2018-10-26 17:51:11 +02:00

View file

@ -7,6 +7,20 @@ let scriptLoading = false;
let scriptLoaded = false; let scriptLoaded = false;
let scriptDidError = false; let scriptDidError = false;
declare class CardVerify {
static stripeHandler: {
open: Function,
close: Function,
};
}
declare class StripeCheckout {
static configure({}): {
open: Function,
close: Function,
};
}
neb-b commented 2018-10-26 04:37:37 +02:00 (Migrated from github.com)
Review

We don't need these. They aren't doing anything as is. Also I belive the declare class syntax is wrong.

We don't need these. They aren't doing anything as is. Also I belive the `declare class` syntax is wrong.
amelzer commented 2018-10-26 09:47:39 +02:00 (Migrated from github.com)
Review

Thank you for your feedback.

The classes are types for the global CardVerify object that you are accessing after adding the script. Not declaring classes for them results in Flow errors

Cannot resolve name CardVerify.
Cannot resolve name StripeCheckout.

As this is a global variable, it would make sense in my opinion to also declare a global class like suggested here: https://flow.org/en/docs/libdefs/creation/#toc-declaring-a-global-class

The goal of my changes was to eliminate the lint & flow errors and therefore I added class definitions for the accessed global objects.

Thank you for your feedback. The classes are types for the global `CardVerify` object that you are accessing after adding the script. Not declaring classes for them results in Flow errors `Cannot resolve name CardVerify.` `Cannot resolve name StripeCheckout.` As this is a global variable, it would make sense in my opinion to also declare a global class like suggested here: https://flow.org/en/docs/libdefs/creation/#toc-declaring-a-global-class The goal of my changes was to eliminate the lint & flow errors and therefore I added class definitions for the accessed global objects.
neb-b commented 2018-10-26 17:50:21 +02:00 (Migrated from github.com)
Review

Ah you are right! I missed the part where we access CardVerify when I first looked this over. Thanks for that link too 🙂 I'll definitely read over that.

Ah you are right! I missed the part where we access `CardVerify` when I first looked this over. Thanks for that link too 🙂 I'll definitely read over that.
type Props = { type Props = {
disabled: boolean, disabled: boolean,
label: ?string, label: ?string,
@ -27,14 +41,20 @@ type Props = {
// token.id can be used to create a charge or customer. // token.id can be used to create a charge or customer.
// token.email contains the email address entered by the user. // token.email contains the email address entered by the user.
token: string, token: string,
email: string,
}; };
class CardVerify extends React.Component { type State = {
constructor(props) { open: boolean,
};
class CardVerifyComponent extends React.Component<Props, State> {
constructor(props: Props) {
super(props); super(props);
this.state = { this.state = {
open: false, open: false,
}; };
this.onClick = this.onClick.bind(this);
} }
componentDidMount() { componentDidMount() {
@ -65,12 +85,14 @@ class CardVerify extends React.Component {
scriptDidError = true; scriptDidError = true;
scriptLoading = false; scriptLoading = false;
reject(event); reject(event);
this.onScriptError(event); this.onScriptError();
}; };
}); });
const wrappedPromise = new Promise((accept, cancel) => { const wrappedPromise = new Promise((accept, cancel) => {
promise.then(() => (canceled ? cancel({ isCanceled: true }) : accept())); promise.then(() => (canceled ? cancel(new Error({ isCanceled: true })) : accept()));
promise.catch(error => (canceled ? cancel({ isCanceled: true }) : cancel(error))); promise.catch(
error => (canceled ? cancel(new Error({ isCanceled: true })) : cancel(error))
);
}); });
return { return {
@ -83,7 +105,9 @@ class CardVerify extends React.Component {
this.loadPromise.promise.then(this.onScriptLoaded).catch(this.onScriptError); this.loadPromise.promise.then(this.onScriptLoaded).catch(this.onScriptError);
document.body.appendChild(script); if (document.body) {
document.body.appendChild(script);
}
} }
componentDidUpdate() { componentDidUpdate() {
@ -112,7 +136,7 @@ class CardVerify extends React.Component {
} }
}; };
onScriptError = (...args) => { onScriptError = () => {
throw new Error('Unable to load credit validation script.'); throw new Error('Unable to load credit validation script.');
}; };
@ -120,6 +144,23 @@ class CardVerify extends React.Component {
this.setState({ open: false }); this.setState({ open: false });
}; };
onClick = () => {
if (scriptDidError) {
throw new Error('Tried to call onClick, but StripeCheckout failed to load');
} else if (CardVerify.stripeHandler) {
this.showStripeDialog();
} else {
this.hasPendingClick = true;
}
};
loadPromise: {
promise: Promise<any>,
cancel: Function,
};
hasPendingClick: boolean;
updateStripeHandler() { updateStripeHandler() {
if (!CardVerify.stripeHandler) { if (!CardVerify.stripeHandler) {
CardVerify.stripeHandler = StripeCheckout.configure({ CardVerify.stripeHandler = StripeCheckout.configure({
@ -142,18 +183,6 @@ class CardVerify extends React.Component {
}); });
} }
onClick = () => {
if (scriptDidError) {
try {
throw new Error('Tried to call onClick, but StripeCheckout failed to load');
} catch (x) {}
} else if (CardVerify.stripeHandler) {
this.showStripeDialog();
} else {
this.hasPendingClick = true;
}
};
render() { render() {
return ( return (
<Button <Button
@ -161,10 +190,10 @@ class CardVerify extends React.Component {
label={this.props.label} label={this.props.label}
icon={icons.LOCK} icon={icons.LOCK}
disabled={this.props.disabled || this.state.open || this.hasPendingClick} disabled={this.props.disabled || this.state.open || this.hasPendingClick}
onClick={this.onClick.bind(this)} onClick={this.onClick}
/> />
); );
} }
} }
export default CardVerify; export default CardVerifyComponent;