Replace usage of old node:url api with WHATWG one in pushgateway.js#744
Conversation
5e0afa6 to
88ee2ba
Compare
Signed-off-by: Yvon Morice <yvon.morice@winamax.fr>
Signed-off-by: Yvon Morice <yvon.morice@winamax.fr>
c8c2194 to
1079eb3
Compare
There was a problem hiding this comment.
Still hasn't removed the old url, just uses it less.
Until the http request is being made with URL I don't believe this PR achieves the goal of replacing the legacy url parser.
Also this PR will need pinning tests.
So what I'd want to see is:
- no urls, full stop
- No manual interpolation. I'm a stickler about this an every time I haven't been has turned into a security hole. And you shouldn't need it at any rate if we stop going back and forth between.
- splitting the URL and the request Options before calling
http.request(url, options)removes the old format and any translation issues lurking there, and also solves the no interpolation problem handily.
This is a Kernighan's Law situation. It's a "there are no obvious bugs" vs "there are obviously no bugs" (Hoare) scenario. The new version should be obviously correct or we haven't really fixed the hole, just punched new ones.
Signed-off-by: Yvon Morice <yvon.morice@winamax.fr>
There was a problem hiding this comment.
Great work. Thanks for the contribution.
I'll take a bit longer perusal of this later today but this looks good on first scan.
Edit: Can you hit the changelog? I know it's kind of a pain. We're thinking about how else to handle it but that's the setting for now.
Related to #743
Replaces usage of
url.urlParseandurl.resolvefromnode:urlbynew url.Urlfrom same package as these functions have been deprecated and trigger a warning on usage.Changes based on code and documentation available at https://nodejs.org/api/url.html#urlresolvefrom-to and https://github.com/nodejs/userland-migrations/tree/main/recipes/node-url-to-whatwg-url