Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

just-camel-case not a drop-in replacement for lodash.camelcase #111

Closed
heygrady opened this issue Oct 15, 2018 · 2 comments · Fixed by #112
Closed

just-camel-case not a drop-in replacement for lodash.camelcase #111

heygrady opened this issue Oct 15, 2018 · 2 comments · Fixed by #112
Assignees

Comments

@heygrady
Copy link

There are some cases where just-camel-case produces subtly different results from lodash.camelcase. I noticed this while trying to replace lodash.camelcase with this library (redux-utilities/redux-actions#331).

I made a quick comparison: https://repl.it/@heygrady/camelcase

Sadly, just-camel-case returns different results when the initial string is in all caps. I tried adding the strict option but then it failed when the initial string was already in camelcase.

  1. It feels like strict mode should be the default (but that's not such a big deal)
  2. Strict mode needs to check if the input string is already in camelcase

The to-camel-case lib handles this by detecting if a word is in camelCase and splits it. From there the join logic you already have would work.

Here (https://repl.it/@heygrady/just-camel-case) is a naiive combination of the logic from to-camel-case with what just-camel-case does:

const wordSeparators = /[\s\u2000-\u206F\u2E00-\u2E7F\\'!"#$%&()*+,\-.\/:;<=>?@\[\]^_`{|}~]+/;
const hasCamel = /([a-z][A-Z]|[A-Z][a-z])/;
const camelSplitter = /(.)([A-Z]+)/g;

function uncamelize(str) {
  return str.replace(camelSplitter, function (m, previous, uppers) {
    return previous + ' ' + uppers.toLowerCase().split('').join(' ');
  }).split(' ');
}

function camelCase(str, options) {
  var words = str.split(wordSeparators);
  var len = words.length;
  var strict = options && options.strict;
  var mappedWords = new Array(len);
  for (var i = 0; i < words.length; i++) {
    var word = words[i];
    if (word === '') {
      continue;
    }
    if (strict && hasCamel.test(word)) {
      var camelWords = uncamelize(str);
      Array.prototype.splice.apply(words, [i, 1].concat(camelWords));
      word = words[i];
    }
    var firstLetter = word[0];
    firstLetter =
      i > 0 ?
        firstLetter.toUpperCase() :
        strict ? firstLetter.toLowerCase() : firstLetter;
    mappedWords[i] = firstLetter + (strict ? word.slice(1).toLowerCase() : word.slice(1));
  }
  return mappedWords.join('');
}
@angus-c
Copy link
Owner

angus-c commented Oct 16, 2018

Many thanks for your observations and feedback Grady. Yeah, I made those decisions consciously, knowing that they differ from lodash etc., but believing that was a more accurate interpretation of the camel case definitions I could find.

However I think you're probably right, it would be more practical to make it behave more like lodash etc. as this has become the de facto standard.

Thanks for your suggestion, I'll use it as the basis for a 'fix' (that will become a new major version) and I think I will get rid of the strict option.

I'm a bit under the weather at the moment, but when I'm ready I plan to use you suggestions but with a few changes

  1. try to include non-english characters in the camel case and splitter regex
  2. change the camel case tester to expect a lower case letter at the start
  3. try to make to code more compact for minimal bytes.

Feel free to put up a PR if you get to it first
Thanks again!

@angus-c angus-c self-assigned this Oct 16, 2018
@angus-c angus-c mentioned this issue Oct 20, 2018
@angus-c
Copy link
Owner

angus-c commented Oct 20, 2018

just-camel-case@4.0.2 fixes these issues. I've dropped the strict option requirement and it now gives very similar results to lodash and passes your tests.

Let me know if I missed anything.

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

Successfully merging a pull request may close this issue.

2 participants