Refactor: JS substr() is deprecated, using slice() instead

[Pranav-yadav]

Summary:

Fixes: #37135

• substr() is not part of the core JS since ~2018
• No wonder why no one noticed this :)
• Though its supported by modern browsers, its deprecated
• Reference: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/substr

Why slice() and not substring()?

Beacuse I like pizza ;) jk

The reason, that I'm not using the most obvious alternative substring() is;

• It swaps the args passed, when; startIndex > endIndex —which I think is not a property of good fn
  and also does not reflects the same in it's name.
• It doesn't support negative args, which I think reduces flexibility to work with it.
• It could lead to more bugs in most of the cases.

Refecrences:

• Ref: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/substring#differences_between_substring_and_slice
• Ref. for slice(): https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/slice
• Ref. for substring(): https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/substring

Changelog:

[GENERAL][FIXED] - Refactor: substr() is deprecated, using slice() instead across RN codebase

Test Plan:

• yarn lint && yarn flow && yarn test-ci --> should be green

[Pranav-yadav]

Note: I haven't applied this change to some generated files e.g. https://github.com/facebook/react-native/blob/30da58733651b9c1e4104c17e37f46ace00b95a7/tools/eslint/rules/sort-imports.js

Seems, it needs to be regenerated at InternalFB side.

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,627,186	-90
android	hermes	armeabi-v7a	7,939,443	-86
android	hermes	x86	9,113,989	-80
android	hermes	x86_64	8,968,858	-75
android	jsc	arm64-v8a	9,191,743	-25
android	jsc	armeabi-v7a	8,381,454	-13
android	jsc	x86	9,249,909	-14
android	jsc	x86_64	9,508,409	-21

Base commit: 243a148
Branch: main

[jacdebug]

I know this is a WIP but just let you know that there is API difference between two,

• substr - Returns a substring based on a starting index and the length of the substring.
• slice - Returns a portion of the string starting from the specified start index and up to but not including the specified end index.

[Pranav-yadav]

Yup, also the reason that I'm not using the most obvious alternative substring() is; it swaps the args passed when;
startIndex > endIndex, which I think is not a property of good fn and also does not reflects in it's name and could lead to more bugs in most of the cases also, it doesn't support negative args, which I think reduces flexibility to work with it.

[Pranav-yadav]

Looks like LogBox expects the specific behavior of substr in it's test-suite, can anyone just confirm? Hence, failing the tests I guess.

Nevermind, I overlooked the obvious thing because, the variable names were confusing 🥲

--

I know this is a WIP but just let you know that there is API difference between two,

@jacdebug done, also rebased!

[facebook-github-bot]

@jacdebug has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[Pranav-yadav]

There are no tests in place for utility/helper functions. I forgot to modify the second param and still all tests passed in CI, such changes could be easily missed by anyone. #37136 (comment)

For each utility/helper function there should be test in-place. cc: @cortinico / @jacdebug WDYT? Should we work on increasing the test-coverage for utility/helper functions?

If there is no code coverage, testing the console as a whole entity is definitely better than testing each individual simple function. This way, the tests written should remain valid even after refactoring the source code. #37136 (comment)

Yeah, exactly, I was trying to convey that only. Testing them as "whole entity" is the way to go, to avoid individual tests and ensure everything works as intended; even after refactors like this one ;)

I'll try to come back to this, after I finish with my Uni. Exams 😅.
Thanks

[facebook-github-bot]

@jacdebug merged this pull request in 8a49754.