refactor: replace deprecated String.substr with String.slice #7662
Conversation
Fixes plotly#6078 String.prototype.substr is deprecated and should be replaced with String.prototype.slice or String.prototype.substring. This commit replaces all occurrences of .substr() with .slice() across the codebase. The slice method is preferred as it handles negative indices more intuitively and matches the existing usage patterns.
- Fixes test failure in svg_text_utils_test.js where slice(0, negative) returned a string instead of empty string (unlike substr). - Fixes similar potential issues in rangeslider/defaults.js and cartesian/axes.js using Math.max(0, ...).
camdecoster
left a comment
camdecoster
left a comment
There was a problem hiding this comment.
Thanks for the PR! It looks like there a handful of build files that also use substr. Could you update those as well? You can ignore anything in stackgl_modules. Other than that, this looks good.
Fixed remaining deprecated substr() calls in build/test files: - test/strict-d3.js - test/jasmine/assets/domain_ref_components.js - test/jasmine/bundle_tests/no_webgl_test.js - test/jasmine/assets/custom_assertions.js - test/jasmine/bundle_tests/plotschema_test.js Excluded stackgl_modules as requested.
|
Done! Found and fixed all the remaining substr calls in the build files (excluding stackgl_modules like you mentioned). Updated 5 files total. |
|
Hey, while fixing those I noticed there are a few more substr calls in test/jasmine/tests/ (like snapshot_test.js, toimage_test.js, axes_test.js, etc.). Want me to update those too or are those fine to leave as-is? |
|
Ah, I had those filtered from my search. Yeah, go ahead. Might as well clean it all up. |
|
Just curious: why did you go with |
|
swapped substr to substring only because of a quick search/replace, not because of a deliberate choice. There’s no functional reason for substring here — shall I update these to slice as well to keep everything consistent ? |
|
Go ahead and use |
Replaced substring() with slice() in test files to maintain consistency with the source code changes where substr() was replaced with slice().
|
One last thing: could you please add a draftlog? |
|
I added a draftlog, Is everything good now? |
|
Thanks for the contribution! There are plenty of other opportunities to update the syntax or switch from deprecated methods. We'd love to have more help getting things modernized. |
2 participants
Fixes #6078
Summary
This PR replaces all occurrences of the deprecated
String.prototype.substr()method withString.prototype.slice()across the codebase.Changes
.substr()with.slice()in 34 filesWhy
sliceoversubstring?slicehandles negative indices intuitivelyslicematches the existing usage patterns in the codebase (e.g.,substr(0, length-1)→slice(0, -1))sliceis the modern recommended replacement forsubstrTesting
npm run lint)