Remove lodash, accepts numbers, consistent return.

[chrisblossom]

This PR removes lodash. There isn't any mutation of the type object so it does not need to be cloned. There is no need to clone() an object that is being pushed onto an array.

The other thing lodash did was force string input. Instead, this returns a match as expected. Maintains consistent returns/inputs.

For the same idea of consistent inputs/returns, I removed an empty string '' from returning the types object. It should return what any un-matched input should.

Removed isAmex from types as it was not consistent with the rest of the results and unnecessary with the .type field.

Fixed running eslint error.

[EvanHahn]

Thank you for the pull request! This PR has a lot of pieces to it, and we (unfortunately) can't accept all of them.

Let's start with the bad news, of things we can't accept:

1. getCardTypes('') needs to return all of the card types, not the empty set. This library is intentionally optimistic; that is, we start with all the possible card types a nd remove them only when "disproven". This was a design descision that works well in our use case.
2. We want to clone the result here. We don't need to clone for the sake of this library's code, but we do want to clone for consumers of the library. One piece of code could u se this library, get a result, and start mutating the values (changing the name or adding new properties, for example)—they might do this innocently. Then, when another piece of code uses this library later, they'd get mutated results.
3. The inputs should always be strings. While credit card numbers are called numbers, they are actually a string of digits. While we currently do not validate any card number s beginning with 0, there is no reason that they cannot begin with 0. Also, Number.MAX_SAFE_INTEGER in many JavaScript runtimes is only 16 digits in length, and there are card numbers that have more than 16 digits.
4. We will keep Lodash. While we'd like to remove Lodash for file size, it's very useful for us here because we need to check for string inputs and we need to do deep clones of objects.

There were some excellent bits to this PR, too:

1. isAmex is probably something we should remove. Its removal is a breaking change so it requires a major version bump.
2. ESLint is borked in this library and we need to fix it. We think this happened because we had optimistic dependency versioning, which bit us here. We'll fix this on our own.

We're going to close this PR, but thanks for your hard work! Sorry to shoot a lot of it down.

[chrisblossom]

No problem at all. In response: 1- I agree, all cards should be returned when nothing is matching. My mistake, should have thought that through more. 2- I think you are misunderstanding what clone() is doing. It is NOT making the data you are returning immutable. An example of when you might use clone would be if you input an object and want to return a modified version of that object. In that case, you would use clone to maintain the object's original state while returning a new object.

Here is an example with your current test (which is not testing anything right now):

  it('preserves integrity of returned values', function () {
    var result = getCardType('4111111111111111');
    result.type = 'whaaaaaat';

    expect(result.type).to.equal('visa'); // <-- result is mutated. test fails.

    expect(getCardType('4111111111111111')[0].type).to.equal('visa'); // <-- new call. nothing to do with result
  });

npm run test
...
  127 passing (38ms)
  1 failing

  1) getCardType preserves integrity of returned values:

      AssertionError: expected 'whaaaaaat' to equal 'visa'
      + expected - actual

      -whaaaaaat
      +visa

The module does not export var types = { ... }; so that being mutated is not a concern.

3- I don't understand why this library cares if the data is a string or not. All you are wanting it to do is return you card type, shouldn't need to force datatypes on someone- although I agree they should be strings.

But if you are going to be opinionated with strings, why not be opinionated and disallow String objects? Those are almost always bad practice.

Also an input that is not a string should return an error of some sort. Throwing is a bad idea, but new Error('only strings accepted') would be fine.

4- At least remove clone. It literally is doing nothing in/for this library.

[chrisblossom]

I've made these changes in https://github.com/chrisblossom/credit-card-type If you would like me to put a PR in again please let me know. I don't mind doing a partial either.

[EvanHahn]

We may be talking past one another on the cloning issue—I don't think I explained it properly. Here's the problem we're trying to avoid with cloning:

var getCardTypes = require('credit-card-type')

// Change something about the returned value.
var visaType = getCardTypes('4111')[0]
visaType.niceType = 'NOT a Visa!! Ha ha!!'

// What does this output?
console.log(getCardTypes('41')[0].niceType)

Without cloning, it outputs 'NOT a Visa!! Ha ha!!'—not what we want. With cloning (the way the library is today), that log outputs 'Visa' as expected.

With respect to strings and types: I think we're in agreement about the "happy path"—when someone passes a regular JavaScript string, everything is fine. When something else is passed, what should happen? We have a few options:

1. do nothing—let it fail however it will
2. return [] (which is what we currently do)
3. throw an error (like you suggested)
4. try to coerce it to a string, which might fail

We opted for ♯2, but any of these options are valid.

We could disallow String objects. Because we're already including Lodash, we thought it makes sense to use its isString functionality which catches some weird edge cases like this. Was there a particular issue that the current functionality is causing?

[chrisblossom]

Please try it without cloning. You are pushing the type object onto a new array and you are not caching any results so they are computed everytime. My PR has clone() removed with that exact test in place.

var getCardTypes = require('credit-card-type')

// Change something about the returned value.
var visaType = getCardTypes('4111')[0]
visaType.niceType = 'NOT a Visa!! Ha ha!!'

// What does this output?
console.log(getCardTypes('41')[0].niceType) // 'visa'

[EvanHahn]

I just ran this in your repo and I got the modified output:

var getCardTypes = require('./')

var visaType = getCardTypes('4111')[0]
visaType.niceType = 'NOT a Visa!! Ha ha!!'

console.log(getCardTypes('41')[0].niceType)  // => 'NOT a Visa!! Ha ha!!'

Our approach is definitely slower because it has to clone every time, but we felt that this was a reasonable tradeoff.

I totally understand if you don't agree with some of the decisions we've made with this library—we've certainly forked and rewritten our fair share of libraries!

[crookedneighbor]

It looks like the test that the integrity of the data stayed in tact from cloning was not working properly. It was changing .type on the array, not on a value in the array. See https://github.com/braintree/credit-card-type/pull/14

[chrisblossom]

@EvanHahn I just don't like the lodash dependency because I don't really feel it is needed.

You guys are right about the var type = being mutated. Definitely learned something new about how objects are "linked" even within different objects. Been too spoiled lately by using the spread operator.

I quickly came up with two other solutions that should work the same as clone() using native JS: https://github.com/chrisblossom/credit-card-type/blob/JSON.parse/index.js#L111 https://github.com/chrisblossom/credit-card-type/blob/getTypes_fn/index.js#L3

Last thing would be to just replace isString with typeof string. Unless you really care about string type objects. Could be solved with a simple one/two liner without a lodash dependency too.

[chrisblossom]

I ended up pushing the result onto a new object. if you would like to see a corresponding PR let me know.

https://github.com/chrisblossom/credit-card-type/blob/d5992c4c033a89fe01b40d8f091590da819dec83/index.js#L132