url.parse() is deemed insecure and has been deprecated.

[oelderinkX]

url.parse() calls have been replaced with new URL.

See documentation on deprecation at:

• https://nodejs.org/api/deprecations.html#dep0169-insecure-urlparse
• https://nodejs.org/dist/latest-v14.x/docs/api/url.html#url_url_parse_urlstring_parsequerystring_slashesdenotehost

[oelderinkX]

@microsoft-github-policy-service agree

[oelderinkX]

/azp run

[azure-pipelines]

Commenter does not have sufficient privileges for PR 662 in repo microsoft/azure-devops-node-api

[oelderinkX]

/azp run

I'm wondering if the pipeline isn't running because my branch name possibly ?

[tarunramsinghani]

/azp run

[oelderinkX]

/azp run

let me know if I need to fix anything on my side

[oelderinkX]

/azp run

[oelderinkX]

do I need to convert my pull request to Draft for it to be review ?

[oelderinkX]

@tarunramsinghani would you have time to review ? I have made the changes you suggested

[oelderinkX]

@tarunramsinghani @adeolemon @peterblazejewicz @nguerrera is anyone available for a review and pull request approval ? I'm new at pull requests for other peoples repos, so apologies in advance

[tarunramsinghani]

/azp run

[tarunramsinghani]

@oelderinkX I am bit pressed for time, so could not spend time to review...

I wanted to make sure this does not have any implications specially with the behaviour change i.e. url.parse returned null in case of invalid url but URL() throws exception so validating those will need some additional test cases or handling in the code.

[oelderinkX]

@oelderinkX I am bit pressed for time, so could not spend time to review...

I wanted to make sure this does not have any implications specially with the behaviour change i.e. url.parse returned null in case of invalid url but URL() throws exception so validating those will need some additional test cases or handling in the code.

Thanks @tarunramsinghani for your comment. url.parse does not return null for invalid urls, but URL.parse does.

if you:

const pathname1 = url.parse('!').pathname; // pathname1 === '!'
const pathname2 = url.parse('www.example.com').pathname; // pathname2 === 'www.example.com'

and you'll be hitting an error somewhere else in the code. If you wanted to switch to URL.parse (you might need to update your typescript + settings) you'd need to do something like:

const pathname = URL.parse('invalid')?.pathname || '';

and you'd still be hitting an error somewhere else.

I'm happy with the Fast-Fail approach here, but are you?. If someone tries setting an invalid Url it will throw a TypeError with a message like:

TypeError: Invalid URL
  at new URL (node:internal/url:797:36)
  at new VsoClient (_build\VsoClient.js:32:25)
  at Context.<anonymous> (test\units\tests.js:51:21)
  at process.processImmediate (node:internal/timers:478:21)

I don't believe a unit test is needed for this change at the moment

[oelderinkX]

anything left to do ?