stripe-angular icon indicating copy to clipboard operation
stripe-angular copied to clipboard

Definite assignment assertion on @Input properties

Open fireflysemantics opened this issue 6 years ago • 2 comments

Hi - Thanks for this awesome project! I've been reviewing the code base and just have a few suggestions that I think apply.

@Input properties

They should not use a definite assignment assertion or !. New angular projects don't allow it. I double checked this with SO here:

https://stackoverflow.com/questions/59093938/angular-input-property-ending-with-exclamation-point''

Also I don't think this should be an @Input property:

@Input() token!:StripeToken

IIUC the component retrieves the Stripe Token via the Stripe API an then emits the value? Thus we never set it on the component ...

ng-container usage

We don't really need an ng-container in the stripe card:

<ng-container *ngIf="!StripeScriptTag.StripeInstance">
          <div style="color:red;">Stripe PublishableKey NOT SET. Use method StripeScriptTag.setPublishableKey()</div>
</ng-container>

IIUC - This will work just as well:

<div *ngIf="!StripeScriptTag.StripeInstance style="color:red;">Stripe PublishableKey NOT SET. Use method StripeScriptTag.setPublishableKey()</div>

ExportAs

Also don't think we need exportAs here:

exportAs:"StripeCard"

It should just export as StripeCard since that is what the component is named.

These things apply to both the stripe-card and the stripe-source.

Thanks again for this sweet project. I'm learning a lot by studying the code base.

fireflysemantics avatar Nov 28 '19 21:11 fireflysemantics

I agree, I think with most everything you have brought up. However, this library was thrown together out of need and I no longer need it. You are welcome to make a pull request for your refinements. In fact I just accepted a very large pull request #23 plus I upgraded us to Angular 9.

Be well.

AckerApple avatar Feb 10 '20 00:02 AckerApple

I have changed jobs to where I needed this library again. I have released a new major update. More work to come.

Hopefully next round of updates I can scoop these recommendations up

AckerApple avatar Jan 05 '21 18:01 AckerApple