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.

Sign up for GitHub

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

Jump to bottom

url,tools,benchmark: replace deprecated substr() #51546

Merged
merged 1 commit into from
May 12, 2024
Merged

url,tools,benchmark: replace deprecated substr() #51546

merged 1 commit into from
May 12, 2024

Conversation

pluris
Copy link
Contributor

@pluris pluris commented Jan 22, 2024 •
edited
Loading

Replace deprecated API.

  • String.prototype.substr() -> String.prototype.substring(), String.prototype.slice()

Refs: https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Global_Objects/String/substr

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/url

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. url Issues and PRs related to the legacy built-in url module. labels Jan 22, 2024
MrJithil
MrJithil approved these changes Jan 23, 2024
View reviewed changes
Copy link
Member

@MrJithil MrJithil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@anonrig
Copy link
Member

anonrig commented Jan 23, 2024

If we are doing this, we should add a ESLint rule to avoid making the same decision over and over again.

@pluris pluris force-pushed the fix/depre_url branch 2 times, most recently from 7242e8c to 879629c Compare January 23, 2024 03:52
@pluris
Copy link
Contributor Author

pluris commented Jan 23, 2024

@anonrig Thank you for your good opinion.
I will update the code about eslint after work.

hulkish
hulkish reviewed Jan 23, 2024
View reviewed changes
lib/url.js Outdated
@@ -958,7 +958,8 @@ Url.prototype.resolveObject = function resolveObject(relative) {
srcPath.unshift('');
}

if (hasTrailingSlash && (srcPath.join('/').substr(-1) !== '/')) {
const srcPathSlashes = srcPath.join('/');
if (hasTrailingSlash && (srcPathSlashes.substring(srcPathSlashes.length - 1) !== '/')) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be srcPathSlashes.charAt(-1) instead of srcPathSlashes.substring(srcPathSlashes.length - 1)?

Copy link
Member

@lpinca lpinca Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

srcPathSlashes.charAt(srcPathSlashes.length -1)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

srcPathSlashes.charAt(srcPathSlashes.length -1)

hmm I mean sure... but .charAt(-1) accomplishes the same result

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Err.. actually just .slice(-1)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.charAt(-1) works differently than before. I think .slice(-1) is correct for this case. Thank you for your opinion.

@pluris
Copy link
Contributor Author

pluris commented Jan 23, 2024

I had divided the commits for review, but I will merge them into one and edit the First commit message.

@pluris pluris force-pushed the fix/depre_url branch 2 times, most recently from 1b33e8a to 07f751c Compare January 23, 2024 23:12
@pluris pluris changed the title url: replace deprecated substr() -> substring() url,tools,benchmark: replace deprecated substr() Jan 23, 2024
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 11, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59115/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59193/

@aduh95 aduh95 merged commit 78dbda1 into nodejs:main May 12, 2024
51 checks passed
@aduh95
Copy link
Contributor

aduh95 commented May 12, 2024

Landed in 78dbda1

targos pushed a commit that referenced this pull request May 13, 2024
@targos
url,tools,benchmark: replace deprecated substr()
ef6a210 
PR-URL: #51546
Refs: https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Global_Objects/String/substr
Reviewed-By: Jithil P Ponnan <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@targos targos mentioned this pull request May 13, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@marco-ippolito
url,tools,benchmark: replace deprecated substr()
f6c8fe4 
PR-URL: #51546
Refs: https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Global_Objects/String/substr
Reviewed-By: Jithil P Ponnan <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@marco-ippolito marco-ippolito mentioned this pull request Jun 17, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@marco-ippolito
url,tools,benchmark: replace deprecated substr()
8056c5a 
PR-URL: #51546
Refs: https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Global_Objects/String/substr
Reviewed-By: Jithil P Ponnan <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@marco-ippolito
url,tools,benchmark: replace deprecated substr()
be9d6f2 
PR-URL: #51546
Refs: https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Global_Objects/String/substr
Reviewed-By: Jithil P Ponnan <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
EliphazBouye pushed a commit to EliphazBouye/node that referenced this pull request Jun 20, 2024
@EliphazBouye
url,tools,benchmark: replace deprecated substr()
456b13c 
PR-URL: nodejs#51546
Refs: https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Global_Objects/String/substr
Reviewed-By: Jithil P Ponnan <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
@bmeck
url,tools,benchmark: replace deprecated substr()
cc3fdea 
PR-URL: nodejs#51546
Refs: https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Global_Objects/String/substr
Reviewed-By: Jithil P Ponnan <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hulkish hulkish hulkish left review comments

@marco-ippolito marco-ippolito marco-ippolito approved these changes

@MrJithil MrJithil MrJithil approved these changes

@lpinca lpinca lpinca approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@pluris @nodejs-github-bot @anonrig @aduh95 @hulkish @lpinca @MrJithil @marco-ippolito