lib: revokeObjectURL throws error on empty args by DylanTet · Pull Request #50433 · nodejs/node

DylanTet · 2023-10-27T18:55:56Z

Fixes: #50432

Added a check to see if url wasnt included as an argument which will then throw an error.

nodejs-github-bot · 2023-10-27T18:56:02Z

Review requested:

@nodejs/url

anonrig

Can you add a test as well?

lib/internal/url.js

DylanTet · 2023-10-27T19:04:14Z

@anonrig yessir ill knock that out

DylanTet · 2023-10-27T22:57:14Z

@anonrig just added the test for the function

test/parallel/test-url-revokeobjecturl.js

jasnell

LGTM with some linting fixes

deokjinkim

First line of commit message has to be started with imperative verb. For example, url: check argument length of revokeObjectURL. Could you please modify first line of commit message?

Refs: https://github.com/nodejs/node/blob/main/doc/contributing/pull-requests.md#commit-message-guidelines

lib/internal/url.js

test/parallel/test-url-revokeobjecturl.js

H4ad · 2023-10-29T13:49:33Z

@DylanTet can you remove the unnecessary commits? The PR was polluted because of them, but in general, it looks great.

DylanTet · 2023-10-29T14:19:35Z

@H4ad just removed the other commits, sorry about that, I am still getting better at git :)

H4ad · 2023-10-29T14:35:40Z

@DylanTet Can you rename the message of the first commit?

https://github.com/nodejs/node/actions/runs/6683924004/job/18160770160?pr=50433

It should be less than 50 characters, maybe something like: lib: revokeObjectURL throws error on empty args.

You should also fix the linting errors: https://github.com/nodejs/node/actions/runs/6683924005/job/18160770724?pr=50433

You can test locally using make lint-js or make lint-md.

test/parallel/test-url-revokeobjecturl.js

lpinca

LGTM with lint errors and commit message title fixed.

shubham9411 · 2023-11-21T05:50:00Z

Hey @DylanTet, Are you working on this?

DylanTet · 2023-11-21T16:31:37Z

@shubham9411 i am, last I remember I updated the PR but it's still waiting review

shubham9411 · 2023-11-21T17:01:17Z

@DylanTet, I think you just need to rename the first commit message as H4ad suggested above.

DylanTet · 2023-11-21T19:58:45Z

@shubham9411 done :)

H4ad · 2023-11-26T13:39:28Z

@DylanTet the first message is still failing the CI, did you forgot to push?

Added a check to see if url wasn't included as an argument which will then throw an error. Fixes: nodejs#50432

nodejs-github-bot · 2023-11-28T14:28:38Z

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

nodejs-github-bot · 2023-11-29T10:05:28Z

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

nodejs-github-bot · 2023-11-30T15:30:44Z

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

nodejs-github-bot · 2023-11-30T21:26:58Z

Landed in 2f40652

Added a check to see if url wasn't included as an argument which will then throw an error. Fixes: #50432 PR-URL: #50433 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>

nodejs-github-bot added needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Oct 27, 2023

anonrig reviewed Oct 27, 2023

View reviewed changes

lib/internal/url.js Outdated Show resolved Hide resolved

lib/internal/url.js Outdated Show resolved Hide resolved

jasnell reviewed Oct 28, 2023

View reviewed changes

test/parallel/test-url-revokeobjecturl.js Outdated Show resolved Hide resolved

jasnell reviewed Oct 28, 2023

View reviewed changes

test/parallel/test-url-revokeobjecturl.js Outdated Show resolved Hide resolved

jasnell approved these changes Oct 28, 2023

View reviewed changes

DylanTet force-pushed the local-branch branch from 9905966 to 3079e71 Compare October 28, 2023 16:57

deokjinkim reviewed Oct 29, 2023

View reviewed changes

lib/internal/url.js Outdated Show resolved Hide resolved

test/parallel/test-url-revokeobjecturl.js Outdated Show resolved Hide resolved

DylanTet changed the title ~~lib: revokeObjectURL throws error if arg length is 0 in url.js~~ url: check argument length of revokeObjectURL Oct 29, 2023

DylanTet force-pushed the local-branch branch from d9f1ca6 to 42a589e Compare October 29, 2023 14:18

H4ad approved these changes Oct 29, 2023

View reviewed changes

H4ad added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Oct 29, 2023

jasnell reviewed Oct 29, 2023

View reviewed changes

test/parallel/test-url-revokeobjecturl.js Show resolved Hide resolved

DylanTet changed the title ~~url: check argument length of revokeObjectURL~~ lib: revokeObjectURL throws error on empty args Oct 30, 2023

DylanTet force-pushed the local-branch branch from 42a589e to 3c4cc75 Compare October 30, 2023 02:10

lpinca approved these changes Oct 30, 2023

View reviewed changes

deokjinkim mentioned this pull request Nov 28, 2023

url: revokeObjectURL should throw an error when no parameter is provided #50925

Closed

deokjinkim force-pushed the local-branch branch from 3c4cc75 to fe65184 Compare November 28, 2023 14:17

deokjinkim added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Nov 28, 2023

url: throw error if argument length of revokeObjectURL is 0

5f66408

Added a check to see if url wasn't included as an argument which will then throw an error. Fixes: nodejs#50432

deokjinkim force-pushed the local-branch branch from fe65184 to 5f66408 Compare November 28, 2023 14:21

deokjinkim added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 28, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 28, 2023

himself65 approved these changes Nov 28, 2023

View reviewed changes

H4ad approved these changes Nov 30, 2023

View reviewed changes

H4ad added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 30, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 30, 2023

nodejs-github-bot merged commit 2f40652 into nodejs:main Nov 30, 2023

targos mentioned this pull request Dec 4, 2023

v21.4.0 release proposal #51043

Merged

richardlau mentioned this pull request Mar 25, 2024

v20.12.0 proposal #52212

Merged

Uh oh!

Conversation

DylanTet commented Oct 27, 2023

Uh oh!

nodejs-github-bot commented Oct 27, 2023

Uh oh!

anonrig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DylanTet commented Oct 27, 2023

Uh oh!

DylanTet commented Oct 27, 2023

Uh oh!

Uh oh!

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

deokjinkim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

H4ad commented Oct 29, 2023

Uh oh!

DylanTet commented Oct 29, 2023

Uh oh!

H4ad commented Oct 29, 2023

Uh oh!

Uh oh!

lpinca left a comment

Choose a reason for hiding this comment

Uh oh!

shubham9411 commented Nov 21, 2023

Uh oh!

DylanTet commented Nov 21, 2023

Uh oh!

shubham9411 commented Nov 21, 2023

Uh oh!

DylanTet commented Nov 21, 2023

Uh oh!

H4ad commented Nov 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 28, 2023

Uh oh!

nodejs-github-bot commented Nov 29, 2023

Uh oh!

nodejs-github-bot commented Nov 30, 2023

Uh oh!

nodejs-github-bot commented Nov 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

H4ad commented Nov 26, 2023 •

edited

Loading