chore: replace deprecated String.prototype.substr() #35421

CommanderRoot · 2022-03-18T02:19:58Z

String.prototype.substr() is deprecated so we replace it with String.prototype.slice() which works similarily but isn't deprecated.
.substr() probably isn't going away anytime soon but the change is trivial so it doesn't hurt to do it.

Bug

Related issues linked using fixes #number
Integration tests added
Errors have helpful link attached, see contributing.md

Feature

Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
Related issues linked using fixes #number
Integration tests added
Documentation added
Telemetry added. In case of a feature if it's used or not.
Errors have helpful link attached, see contributing.md

Documentation / Examples

Make sure the linting passes by running yarn lint

.substr() is deprecated so we replace it with .slice() which works similarily but isn't deprecated Signed-off-by: Tobias Speicher <rootcommander@gmail.com>

ijjk · 2022-03-18T10:12:36Z

Failing test suites

Commit: 36601c4

yarn testheadless test/integration/custom-routes/test/index.test.js

Custom routes > serverless mode > should handle external beforeFiles rewrite correctly

Expand output

● Custom routes › serverless mode › should handle external beforeFiles rewrite correctly

expect(received).toBe(expected) // Object.is equality

Expected: 200
Received: 500

  89 |       console.error('Invalid response', html)
  90 |     }
> 91 |     expect(res.status).toBe(200)
     |                        ^
  92 |     expect(html).toContain('Vercel')
  93 |
  94 |     const browser = await webdriver(appPort, '/nav')

  at Object.<anonymous> (integration/custom-routes/test/index.test.js:91:24)

Read more about building and testing Next.js in contributing.md.

styfle

Note that substr and slice are not exactly the same.

Consider this example:

const str = 'hello';
console.log(str.substr(0, -1));
console.log(str.slice(0, -1));

Also looks like a test is failing

CommanderRoot · 2022-03-18T13:37:04Z

Note that substr and slice are not exactly the same.

Consider this example:
const str = 'hello';
console.log(str.substr(0, -1));
console.log(str.slice(0, -1));
Also looks like a test is failing

This is correct. It can sometimes make sense to use substring() if the second parameter could be negative and you don't want to have all chars until the x last.
In the cases here we're mostly just calling the function with one parameter which results in substr() and slice() returning the same values. In cases where we use the second parameter it's never negative, if that's not wanted.

As for the failed test I'm not sure why that's happening as it did work with the same code earlier, just missing liting. Might be a test fluke?

SukkaW · 2022-03-20T07:51:04Z

please do notice that String.prototype.substr is deprecated because it is replaced by String.prototype.substring, which is not deprecated.

CommanderRoot · 2022-03-20T07:58:53Z

Both String.prototype.substring and String.prototype.slice can be used as replacement. There are however some differences in returned values when using a second parameter. I personally like String.prototype.slice more as it's "stricter" and doesn't sort / switch parameters. There are however use cases where String.prototype.substring is the better choice if you want the same results as String.prototype.substr, for example when the second parameter is normally positive but there is a chance for it to be negative.

styfle

Can we add a rule to eslint? Otherwise we'll likely get substr() usage again since we merge 150+ PRs each month.

CommanderRoot · 2022-03-21T18:09:19Z

I have created #35499 which adds an ESLint warning when someone wants to use substr()

styfle

Thanks!

ijjk · 2022-03-24T21:23:15Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall decrease ✓

	vercel/next.js canary	CommanderRoot/next.js chore/rm-deprecated-substr	Change
buildDuration	16s	15.6s	-443ms
buildDurationCached	6.2s	6.2s	-95ms
nodeModulesSize	467 MB	467 MB	-166 B

Page Load Tests Overall increase ✓

	vercel/next.js canary	CommanderRoot/next.js chore/rm-deprecated-substr	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.205	3.135	-0.07
/ avg req/sec	779.96	797.55	+17.59
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.384	1.364	-0.02
/error-in-render avg req/sec	1806.5	1833.34	+26.84

Client Bundles (main, webpack) Overall decrease ✓

	vercel/next.js canary	CommanderRoot/next.js chore/rm-deprecated-substr	Change
925.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42 kB	42 kB	✓
main-HASH.js gzip	28 kB	28 kB	-5 B
webpack-HASH.js gzip	1.44 kB	1.44 kB	✓
Overall change	71.6 kB	71.6 kB	-5 B

Legacy Client Bundles (polyfills)

	vercel/next.js canary	CommanderRoot/next.js chore/rm-deprecated-substr	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	CommanderRoot/next.js chore/rm-deprecated-substr	Change
_app-HASH.js gzip	1.36 kB	1.36 kB	✓
_error-HASH.js gzip	192 B	192 B	✓
amp-HASH.js gzip	309 B	309 B	✓
css-HASH.js gzip	327 B	327 B	✓
dynamic-HASH.js gzip	2.57 kB	2.57 kB	✓
head-HASH.js gzip	351 B	351 B	✓
hooks-HASH.js gzip	920 B	920 B	✓
image-HASH.js gzip	5.48 kB	5.48 kB	✓
index-HASH.js gzip	263 B	263 B	✓
link-HASH.js gzip	2.26 kB	2.26 kB	✓
routerDirect..HASH.js gzip	320 B	320 B	✓
script-HASH.js gzip	387 B	387 B	✓
withRouter-HASH.js gzip	319 B	319 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	15.2 kB	15.2 kB	✓

Client Build Manifests

	vercel/next.js canary	CommanderRoot/next.js chore/rm-deprecated-substr	Change
_buildManifest.js gzip	460 B	460 B	✓
Overall change	460 B	460 B	✓

Rendered Page Sizes Overall increase ⚠️

	vercel/next.js canary	CommanderRoot/next.js chore/rm-deprecated-substr	Change
index.html gzip	531 B	531 B	✓
link.html gzip	544 B	545 B	⚠️ +1 B
withRouter.html gzip	524 B	525 B	⚠️ +1 B
Overall change	1.6 kB	1.6 kB	⚠️ +2 B

Diffs

Diff for main-HASH.js

@@ -4401,7 +4401,7 @@
         return (
           (0, _normalizeTrailingSlash).normalizePathTrailingSlash(
             "".concat(prefix).concat(pathname)
-          ) + path.substr(pathname.length)
+          ) + path.slice(pathname.length)
         );
       }
       function getDomainLocale(path, locale, locales, domainLocales) {
@@ -4534,7 +4534,7 @@
         // invalid and will never match a Next.js page/file
         var urlProtoMatch = urlAsString.match(/^[a-zA-Z]{1,}:\/\//);
         var urlAsStringNoProto = urlProtoMatch
-          ? urlAsString.substr(urlProtoMatch[0].length)
+          ? urlAsString.slice(urlProtoMatch[0].length)
           : urlAsString;
         var urlParts = urlAsStringNoProto.split("?");
         if ((urlParts[0] || "").match(/(\/\/|\\)/)) {
@@ -4878,7 +4878,7 @@
           if (true) {
             // make sure "as" doesn't start with double slashes or else it can
             // throw an error as it's considered invalid
-            if (as1.substr(0, 2) !== "//") {
+            if (!as1.startsWith("//")) {
               // in order for `e.state` to work on the `onpopstate` event
               // we have to register the initial route upon initialization
               var options1 = {
@@ -6737,7 +6737,7 @@
           query = String(querystring.urlQueryToSearchParams(query));
         }
         var search = urlObj.search || (query && "?".concat(query)) || "";
-        if (protocol && protocol.substr(-1) !== ":") protocol += ":";
+        if (protocol && !protocol.endsWith(":")) protocol += ":";
         if (
           urlObj.slashes ||
           ((!protocol || slashedProtocols.test(protocol)) && host !== false)

Diff for index.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-0bf777b4298ee871.js"
+      src="/_next/static/chunks/main-356143d159a72188.js"
       defer=""
     ></script>
     <script

Diff for link.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-0bf777b4298ee871.js"
+      src="/_next/static/chunks/main-356143d159a72188.js"
       defer=""
     ></script>
     <script

Diff for withRouter.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-0bf777b4298ee871.js"
+      src="/_next/static/chunks/main-356143d159a72188.js"
       defer=""
     ></script>
     <script

Default Build with SWC (Increase detected ⚠️)

General Overall decrease ✓

	vercel/next.js canary	CommanderRoot/next.js chore/rm-deprecated-substr	Change
buildDuration	19.2s	19s	-202ms
buildDurationCached	6.1s	6.2s	⚠️ +107ms
nodeModulesSize	467 MB	467 MB	-166 B

Page Load Tests Overall increase ✓

	vercel/next.js canary	CommanderRoot/next.js chore/rm-deprecated-substr	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.141	3.133	-0.01
/ avg req/sec	796.03	798.06	+2.03
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.371	1.363	-0.01
/error-in-render avg req/sec	1822.86	1834.46	+11.6

Client Bundles (main, webpack) Overall decrease ✓

	vercel/next.js canary	CommanderRoot/next.js chore/rm-deprecated-substr	Change
925.HASH.js gzip	178 B	178 B	✓
framework-HASH.js gzip	42.3 kB	42.3 kB	✓
main-HASH.js gzip	28.2 kB	28.2 kB	-2 B
webpack-HASH.js gzip	1.45 kB	1.45 kB	✓
Overall change	72.1 kB	72.1 kB	-2 B

Legacy Client Bundles (polyfills)

	vercel/next.js canary	CommanderRoot/next.js chore/rm-deprecated-substr	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	CommanderRoot/next.js chore/rm-deprecated-substr	Change
_app-HASH.js gzip	1.35 kB	1.35 kB	✓
_error-HASH.js gzip	179 B	179 B	✓
amp-HASH.js gzip	313 B	313 B	✓
css-HASH.js gzip	324 B	324 B	✓
dynamic-HASH.js gzip	2.56 kB	2.56 kB	✓
head-HASH.js gzip	351 B	351 B	✓
hooks-HASH.js gzip	921 B	921 B	✓
image-HASH.js gzip	5.59 kB	5.59 kB	✓
index-HASH.js gzip	261 B	261 B	✓
link-HASH.js gzip	2.33 kB	2.33 kB	✓
routerDirect..HASH.js gzip	322 B	322 B	✓
script-HASH.js gzip	388 B	388 B	✓
withRouter-HASH.js gzip	317 B	317 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	15.3 kB	15.3 kB	✓

Client Build Manifests

	vercel/next.js canary	CommanderRoot/next.js chore/rm-deprecated-substr	Change
_buildManifest.js gzip	458 B	458 B	✓
Overall change	458 B	458 B	✓

Rendered Page Sizes

	vercel/next.js canary	CommanderRoot/next.js chore/rm-deprecated-substr	Change
index.html gzip	531 B	531 B	✓
link.html gzip	546 B	546 B	✓
withRouter.html gzip	527 B	527 B	✓
Overall change	1.6 kB	1.6 kB	✓

Diffs

Diff for main-HASH.js

@@ -4401,7 +4401,7 @@
         return (
           (0, _normalizeTrailingSlash).normalizePathTrailingSlash(
             "".concat(prefix).concat(pathname)
-          ) + path.substr(pathname.length)
+          ) + path.slice(pathname.length)
         );
       }
       function getDomainLocale(path, locale, locales, domainLocales) {
@@ -4534,7 +4534,7 @@
         // invalid and will never match a Next.js page/file
         var urlProtoMatch = urlAsString.match(/^[a-zA-Z]{1,}:\/\//);
         var urlAsStringNoProto = urlProtoMatch
-          ? urlAsString.substr(urlProtoMatch[0].length)
+          ? urlAsString.slice(urlProtoMatch[0].length)
           : urlAsString;
         var urlParts = urlAsStringNoProto.split("?");
         if ((urlParts[0] || "").match(/(\/\/|\\)/)) {
@@ -4878,7 +4878,7 @@
           if (true) {
             // make sure "as" doesn't start with double slashes or else it can
             // throw an error as it's considered invalid
-            if (as1.substr(0, 2) !== "//") {
+            if (!as1.startsWith("//")) {
               // in order for `e.state` to work on the `onpopstate` event
               // we have to register the initial route upon initialization
               var options1 = {
@@ -6737,7 +6737,7 @@
           query = String(querystring.urlQueryToSearchParams(query));
         }
         var search = urlObj.search || (query && "?".concat(query)) || "";
-        if (protocol && protocol.substr(-1) !== ":") protocol += ":";
+        if (protocol && !protocol.endsWith(":")) protocol += ":";
         if (
           urlObj.slashes ||
           ((!protocol || slashedProtocols.test(protocol)) && host !== false)

Diff for index.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-0bf777b4298ee871.js"
+      src="/_next/static/chunks/main-356143d159a72188.js"
       defer=""
     ></script>
     <script

Diff for link.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-0bf777b4298ee871.js"
+      src="/_next/static/chunks/main-356143d159a72188.js"
       defer=""
     ></script>
     <script

Diff for withRouter.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-0bf777b4298ee871.js"
+      src="/_next/static/chunks/main-356143d159a72188.js"
       defer=""
     ></script>
     <script

Commit: 3783435

* test: warn on substr() usage Don't allow any new substr() usage after #35421 Signed-off-by: Tobias Speicher <rootcommander@gmail.com> * Apply suggestions from code review * Use slice in router-utils Co-authored-by: Steven <steven@ceriously.com>

CommanderRoot requested review from timneutkens, ijjk, shuding, padmaia, huozhi, leerob, steven-tey and styfle as code owners March 18, 2022 02:19

ijjk added example bug Issue was opened via the example bug report template. type: next labels Mar 18, 2022

This comment has been minimized.

Sign in to view

chore: replace deprecated String.prototype.substr()

36601c4

.substr() is deprecated so we replace it with .slice() which works similarily but isn't deprecated Signed-off-by: Tobias Speicher <rootcommander@gmail.com>

This comment has been minimized.

Sign in to view

styfle reviewed Mar 18, 2022

View reviewed changes

Merge branch 'canary' into chore/rm-deprecated-substr

6a98cc2

This comment has been minimized.

Sign in to view

Merge branch 'canary' into chore/rm-deprecated-substr

e5b473b

This comment has been minimized.

Sign in to view

styfle reviewed Mar 21, 2022

View reviewed changes

CommanderRoot mentioned this pull request Mar 21, 2022

test: warn on substr() usage #35499

Merged

styfle approved these changes Mar 21, 2022

View reviewed changes

styfle added 2 commits March 21, 2022 17:58

Merge branch 'canary' into chore/rm-deprecated-substr

2b3b0f8

Merge branch 'canary' into chore/rm-deprecated-substr

3783435

styfle merged commit db2567b into vercel:canary Mar 24, 2022

github-actions bot locked as resolved and limited conversation to collaborators Apr 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: replace deprecated String.prototype.substr() #35421

chore: replace deprecated String.prototype.substr() #35421

CommanderRoot commented Mar 18, 2022

This comment has been minimized.

This comment has been minimized.

ijjk commented Mar 18, 2022

This comment has been minimized.

styfle left a comment

CommanderRoot commented Mar 18, 2022 •

edited

This comment has been minimized.

This comment has been minimized.

SukkaW commented Mar 20, 2022

CommanderRoot commented Mar 20, 2022

styfle left a comment

CommanderRoot commented Mar 21, 2022

styfle left a comment

ijjk commented Mar 24, 2022

Diffs

Diffs

chore: replace deprecated String.prototype.substr() #35421

chore: replace deprecated String.prototype.substr() #35421

Conversation

CommanderRoot commented Mar 18, 2022

Bug

Feature

Documentation / Examples

This comment has been minimized.

This comment has been minimized.

ijjk commented Mar 18, 2022

Failing test suites

This comment has been minimized.

styfle left a comment

Choose a reason for hiding this comment

CommanderRoot commented Mar 18, 2022 • edited

This comment has been minimized.

This comment has been minimized.

SukkaW commented Mar 20, 2022

CommanderRoot commented Mar 20, 2022

styfle left a comment

Choose a reason for hiding this comment

CommanderRoot commented Mar 21, 2022

styfle left a comment

Choose a reason for hiding this comment

ijjk commented Mar 24, 2022

Stats from current PR

Diffs

Diffs

CommanderRoot commented Mar 18, 2022 •

edited