Replace the payment dependency with credit-card-type and luhn #77

cassiocardoso · 2020-05-02T09:13:32Z

Proposal

Remove Payment dependency in favor of credit-card-type + luhn.

Closes #75

Details

After removing Payment, we needed to store the list of supported card types inside the library, I chose to do that using the internal component state.

Some small tweaks were needed since the libs identified the card types differently:

amex -> american-express
dinersclub -> diners-club
visaelectron -> visa-electron (this one was just for consistency 😅 )

No changes were made to the component API.

Caveats

Currently, Dankort is not supported by credit-card-type which led to some inline changes here to make it work.

If/when this PR is approved: braintree/credit-card-type#106 we'll be able to remove these changes on our side and rely on it

cassiocardoso · 2020-05-02T09:15:52Z

src/cardTypes.js

@@ -0,0 +1,35 @@
+export const dankort = {


I created these 3 definitions here while they aren't supported by credit-card-type

As we discussed, use this file (with a different name) to abstract the usage of credit-card-type and luhn so it will be easy to replace it in the future if we need to.

I moved this file to /utils.

Initially, I had the helpers and these objects in the same file, but it was a bit confusing for me, so I decided to split into 2 files just to be easier to understand what's being done in each of them.

Let me know what do you think.

src/index.js

cassiocardoso · 2020-05-02T09:21:34Z

src/index.js

+    if (number && !preview) {
+      const validatedIssuer = creditCardType(sanitizeNumber(number)).length ? creditCardType(sanitizeNumber(number))[0].type : 'unknown';
+
+      if (validCardTypes.includes(validatedIssuer)) {
+        updatedIssuer = validatedIssuer;
+      }
+    }


I had to tweak the logic here since creditCardType will always return an array of card type objects.

Also added the option to bypass this logic if the preview prop is passed, in this case, we use the issuer prop

Another reason for the abstraction.
Get the issuer only if there's a single item in the array.

and we already have an issuer getter that handles that.

got your point on the issuer getter, makes total sense, reverted the logic here.

But I noticed one thing with this part:

Get the issuer only if there's a single item in the array.

In this case, we get a lot more unknowns, for example:

number type

4 unknown

40 unknown

401 unknown

4011 unknown

40117 unknown

401176 visa

401178 elo

Before we got visa from 4 until 40117, and only switched to elo if the bin matched. Do you think this could be an issue?

src/utils.js

README.md

package.json

src/index.js

gilbarbara

A few changes

README.md

package.json

README.md

gilbarbara · 2020-05-02T16:15:38Z

src/cardTypes.js

@@ -0,0 +1,35 @@
+export const dankort = {


As we discussed, use this file (with a different name) to abstract the usage of credit-card-type and luhn so it will be easy to replace it in the future if we need to.

gilbarbara · 2020-05-02T16:29:56Z

src/index.js

+    if (number && !preview) {
+      const validatedIssuer = creditCardType(sanitizeNumber(number)).length ? creditCardType(sanitizeNumber(number))[0].type : 'unknown';
+
+      if (validCardTypes.includes(validatedIssuer)) {
+        updatedIssuer = validatedIssuer;
+      }
+    }


Another reason for the abstraction.
Get the issuer only if there's a single item in the array.

and we already have an issuer getter that handles that.

src/index.js

gilbarbara · 2020-05-02T16:35:42Z

src/styles.scss

@@ -26,9 +26,9 @@ $rccs-background-transition: all 0.5s ease-out !default;
 $rccs-animate-background: true;

 /** ISSUERS **/
-$rccs-amex-background: linear-gradient(25deg, #308c67, #a3f2cf) !default;


we might need to revert these

src/utils.js

gilbarbara · 2020-05-02T16:36:44Z

test/index.spec.js

@@ -78,11 +78,11 @@ describe('ReactCreditCards', () => {
      focused: 'number',
    });

-    expect(wrapper.find('.rccs__card').hasClass('rccs__card--amex')).toBe(true);


The same thing here, we might need to revert these changes

I'm not sure if I got your point here, but given the newly created cardTypesMap:

export const cardTypesMap = { amex: ['amex', 'americanexpress', 'american-express'], dinersclub: ['diners', 'dinersclub', 'diners-club'], visaelectron: ['visaelectron', 'visa-electron'], };

Should we use the object keys as our identifiers for the CSS class?

- use credit-card-type + luhn to achieve the same functionality previously provided by the payment lib - some types had their names updated to match the new lib pattern (american-express and diners-club)

- add new card type information - update local development section

- use class getter and setter to store the validCardTypes value instead of using the component state - refactor config function name to be more explicit about its implementation

- fix issue reported by DeepScan

jonyw4 · 2020-08-19T12:44:28Z

any progress in this pr?

AnthonyCrowcroft · 2021-11-01T07:37:10Z

@cassiocardoso and @gilbarbara was there anything left outstanding on this it'd be super cool to see this unit finished :)

cassioseffrin · 2022-03-12T17:51:37Z

This PR seems to be very useful. Any estimative when it will be merged it with master?