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

splitWordsRegex fails on iOS 16 #80

Closed
acorn1010 opened this issue Apr 1, 2023 · 6 comments · Fixed by #86
Closed

splitWordsRegex fails on iOS 16 #80

acorn1010 opened this issue Apr 1, 2023 · 6 comments · Fixed by #86

Comments

@acorn1010
Copy link

acorn1010 commented Apr 1, 2023

Safari iOS below 16.3 doesn't support certain group specifiers in the below RegEx, probably lookbehind: https://caniuse.com/?search=lookbehind.

const splitWordsRegex = new RegExp(
    '[^\\dA-Za-z]' +  // match any character that is not a letter or a digit
    '|' +                    // or
    '(?<=[a-z])' +           // lookbehind for a lowercase letter
    '(?=[A-Z])' +            // lookahead for an uppercase letter
    '|' +                    // or
    '(?<=[A-Z])' +           // lookbehind for an uppercase letter
    '(?=[A-Z][a-z])'         // lookahead for an uppercase letter followed by a lowercase letter
);

I haven't fully tested it, but what about replacing this splitWords function with something like:

const splitWordsRegex = /([\dA-Z][\da-z]+)|([\dA-Z]+)|([\da-z]+)/g;

export function splitWords(str: string): string[] {
  return [...str.matchAll(splitWordsRegex)].map(match => match[0]);
}

Other issues I've encountered with switching from Lodash to moderndash on a medium-size project:

  • range(0, 3) is inclusive instead of exclusive (e.g. returning [0, 1, 2, 3] instead of [0, 1, 2]). This is different from Lodash, and resulted in some bugs.
  • merge({}, b) doesn't modify the first input. While it's typically best practice to not modify inputs, I believe it makes sense here because otherwise a shallow copy (e.g. {...{}, ...b}) could be used in most cases. I've personally used Lodash's merge in areas of my code specifically because it modifies the input, cutting down on garbage collection.
@Maggi64
Copy link
Owner

Maggi64 commented Apr 4, 2023

Hi @acorn1010, thanks for the suggestion. I will answer this step by step. Your regex doesn't account for this case:

AssertionError: expected 'some_cruelw_orld' to be 'some_cruel_world' // Object.is equality
 ❯ test/string/snakeCase.test.ts:10:45
     10|         expect(snakeCase('someCRUELWorld')).toBe('some_cruel_world');
       |                                             ^
     11|     });
     12|

  - Expected   "some_cruel_world"
  + Received   "some_cruelw_orld"

But I agree that we should at least make it compatible with Safari 15. I will see what good replacements are out there when I have the time.

@Maggi64
Copy link
Owner

Maggi64 commented Apr 4, 2023

range(0, 3) is inclusive instead of exclusive (e.g. returns [0, 1, 2, 3] instead of [0, 1, 2]). This is different from Lodash and caused some bugs.

I decided to do this to have a more readable syntax. All ranges in moderndash include the last parameter. In randomInt and randomFloat the last parameter is also included in the range.
ModernDash is not intended to be a drop-in replacement. Instead, I tried to make the syntax as readable as possible. Therefore I would like to keep the range function as it is.

@Maggi64 Maggi64 linked a pull request Apr 5, 2023 that will close this issue
@Maggi64
Copy link
Owner

Maggi64 commented Apr 5, 2023

@acorn1010

The issue got autoclosed with merging the PR.
I added a fallback for browsers that don't support positive lookbehind in regex. Let me know if it works! :)

We can still discuss if it makes sense to modify inputs in merge.

@acorn1010
Copy link
Author

acorn1010 commented Apr 6, 2023

Sorry about the original RegEx—just noticed there were tests! This one should cover everything in the tests, plus some other edge cases like "enable6HFormat" -> ["enable", "6H", "Format"] while still maintaining a small file size:

const splitWordsRegex = /(\d*[a-z]+)|([A-Z][a-z]+)|(\d*[A-Z]+(?=[^a-z]|$))|(\d+)/g;

(I got the enable6HFormat from here: https://github.com/lodash/lodash/blob/4.17.11/test/test.js#L2351)

P.S. The fallback works!

@Maggi64
Copy link
Owner

Maggi64 commented Apr 6, 2023

Thanks, this is a much simpler solution. I deployed it in 3.3.1. It is 10% slower than my lookbehind solution, but the
"enable6HFormat" -> ["enable", "6H", "format"] case was handled differently. It makes sense to match lodash's behaviour and its still 2x faster. 🥳

@Maggi64
Copy link
Owner

Maggi64 commented Apr 7, 2023

@acorn1010 opened a new issue for the array mutations, where we can discuss this further. ⬆️

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