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

doc: clarify treatment of non-string argument to new URL() #41658

Merged
merged 2 commits into from
Jan 25, 2022
Merged

doc: clarify treatment of non-string argument to new URL() #41658

merged 2 commits into from
Jan 25, 2022

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 23, 2022

Closes: #41653

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. url Issues and PRs related to the legacy built-in url module. labels Jan 23, 2022
@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 23, 2022
TimothyGu
TimothyGu reviewed Jan 23, 2022
View reviewed changes
doc/api/url.md Outdated Show resolved Hide resolved
@Trott @TimothyGu
Update doc/api/url.md
ff5b5a4 
Co-authored-by: Timothy Gu <[email protected]>
Trott
Trott commented Jan 23, 2022
View reviewed changes
doc/api/url.md
@@ -131,7 +131,7 @@ return `true`.

* `input` {string} The absolute or relative input URL to parse. If `input`
is relative, then `base` is required. If `input` is absolute, the `base`
is ignored.
is ignored. If `input` is not a string, it is [converted to a string][] first.
* `base` {string|URL} The base URL to resolve against if the `input` is not
Copy link
Member Author

Choose a reason for hiding this comment

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

base is also USVString so perhaps this should be {string} rather than {string|URL} and also include the note about being converted to a string that is added on line 134 for input.

Copy link
Member Author

Choose a reason for hiding this comment

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

base is also USVString so perhaps this should be {string} rather than {string|URL} and also include the note about being converted to a string that is added on line 134 for input.

@TimothyGu @jasnell Sound OK to you?

@Trott Trott mentioned this pull request Jan 24, 2022
Closed
@Trott Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 25, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 25, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/41658
✔  Done loading data for nodejs/node/pull/41658
----------------------------------- PR info ------------------------------------
Title      doc: clarify treatment of non-string argument to new URL() (#41658)
Author     Rich Trott  (@Trott)
Branch     Trott:newurl -> nodejs:master
Labels     doc, url, author ready
Commits    2
 - doc: clarify treatment of non-string argument to new URL()
 - Update doc/api/url.md
Committers 2
 - Rich Trott 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/41658
Fixes: https://github.com/nodejs/node/issues/41653
Reviewed-By: Richard Lau 
Reviewed-By: Mohammed Keyvanzadeh 
Reviewed-By: Luigi Pinca 
Reviewed-By: Darshan Sen 
Reviewed-By: Mestery 
Reviewed-By: Benjamin Gruenbaum 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41658
Fixes: https://github.com/nodejs/node/issues/41653
Reviewed-By: Richard Lau 
Reviewed-By: Mohammed Keyvanzadeh 
Reviewed-By: Luigi Pinca 
Reviewed-By: Darshan Sen 
Reviewed-By: Mestery 
Reviewed-By: Benjamin Gruenbaum 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 23 Jan 2022 00:21:07 GMT
   ✔  Approvals: 6
   ✔  - Richard Lau (@richardlau) (TSC): https://github.com/nodejs/node/pull/41658#pullrequestreview-860252993
   ✔  - Mohammed Keyvanzadeh (@VoltrexMaster): https://github.com/nodejs/node/pull/41658#pullrequestreview-860254672
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/41658#pullrequestreview-860283743
   ✔  - Darshan Sen (@RaisinTen) (TSC): https://github.com/nodejs/node/pull/41658#pullrequestreview-860316074
   ✔  - Mestery (@Mesteery): https://github.com/nodejs/node/pull/41658#pullrequestreview-860318289
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/41658#pullrequestreview-861643651
   ✔  Last GitHub CI successful
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/master up to date...
From https://github.com/nodejs/node
 * branch                  master     -> FETCH_HEAD
✔  origin/master is now up-to-date
- Downloading patch for 41658
From https://github.com/nodejs/node
 * branch                  refs/pull/41658/merge -> FETCH_HEAD
✔  Fetched commits as b27ae24dcc42..ff5b5a438465
--------------------------------------------------------------------------------
[master fb622bf670] doc: clarify treatment of non-string argument to new URL()
 Author: Rich Trott 
 Date: Sat Jan 22 16:18:30 2022 -0800
 1 file changed, 3 insertions(+), 2 deletions(-)
[master 93ce1361e9] Update doc/api/url.md
 Author: Rich Trott 
 Date: Sat Jan 22 22:30:00 2022 -0800
 1 file changed, 1 insertion(+), 1 deletion(-)
   ✔  Patches applied
There are 2 commits in the PR. Attempting autorebase.
Rebasing (2/4)

Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

doc: clarify treatment of non-string argument to new URL()


Closes: #41653


PR-URL: #41658

Fixes: #41653

Reviewed-By: Richard Lau [email protected]

Reviewed-By: Mohammed Keyvanzadeh [email protected]

Reviewed-By: Luigi Pinca [email protected]

Reviewed-By: Darshan Sen [email protected]

Reviewed-By: Mestery [email protected]

Reviewed-By: Benjamin Gruenbaum [email protected]


[detached HEAD e4abdaf84a] doc: clarify treatment of non-string argument to new URL()

Author: Rich Trott [email protected]

Date: Sat Jan 22 16:18:30 2022 -0800

1 file changed, 3 insertions(+), 2 deletions(-)

Rebasing (3/4)

Rebasing (4/4)


Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

Update doc/api/url.md


Co-authored-by: Timothy Gu [email protected]


PR-URL: #41658

Fixes: #41653

Reviewed-By: Richard Lau [email protected]

Reviewed-By: Mohammed Keyvanzadeh [email protected]

Reviewed-By: Luigi Pinca [email protected]

Reviewed-By: Darshan Sen [email protected]

Reviewed-By: Mestery [email protected]

Reviewed-By: Benjamin Gruenbaum [email protected]


[detached HEAD 4d0563e1a8] Update doc/api/url.md

Author: Rich Trott [email protected]

Date: Sat Jan 22 22:30:00 2022 -0800

1 file changed, 1 insertion(+), 1 deletion(-)


Successfully rebased and updated refs/heads/master.


ℹ  Use --fixupAll option, squash the PR manually or land the PR from the command line.
https://github.com/nodejs/node/actions/runs/1743119502

@benjamingr benjamingr added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jan 25, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 25, 2022
@nodejs-github-bot nodejs-github-bot merged commit 88a3bd5 into nodejs:master Jan 25, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 88a3bd5

@Trott Trott deleted the newurl branch January 25, 2022 02:28
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
@Trott @Linkgoron
doc: clarify treatment of non-string argument to new URL()
6d6631a 
Closes: nodejs#41653

PR-URL: nodejs#41658
Fixes: nodejs#41653
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
ruyadorno pushed a commit that referenced this pull request Feb 8, 2022
@Trott @ruyadorno
doc: clarify treatment of non-string argument to new URL()
986dbb3 
Closes: #41653

PR-URL: #41658
Fixes: #41653
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Feb 8, 2022
Merged
danielleadams pushed a commit that referenced this pull request Mar 2, 2022
@Trott @danielleadams
doc: clarify treatment of non-string argument to new URL()
7533757 
Closes: #41653

PR-URL: #41658
Fixes: #41653
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@danielleadams danielleadams mentioned this pull request Mar 3, 2022
Merged
danielleadams pushed a commit that referenced this pull request Mar 3, 2022
@Trott @danielleadams
doc: clarify treatment of non-string argument to new URL()
f1c0341 
Closes: #41653

PR-URL: #41658
Fixes: #41653
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
danielleadams pushed a commit that referenced this pull request Mar 14, 2022
@Trott @danielleadams
doc: clarify treatment of non-string argument to new URL()
59f95fe 
Closes: #41653

PR-URL: #41658
Fixes: #41653
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimothyGu TimothyGu TimothyGu left review comments

@VoltrexKeyva VoltrexKeyva VoltrexKeyva approved these changes

@Mesteery Mesteery Mesteery approved these changes

@RaisinTen RaisinTen RaisinTen approved these changes

@richardlau richardlau richardlau approved these changes

@lpinca lpinca lpinca approved these changes

@benjamingr benjamingr benjamingr 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. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations. 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.

new URL() accepts array of string
9 participants
@Trott @nodejs-github-bot @benjamingr @lpinca @TimothyGu @richardlau @RaisinTen @Mesteery @VoltrexKeyva