feat: move to built-in URL class #998
Conversation
We currently use `url-parse` to deal with URL construction. Given our node engine constraint of `>=14`, we know `URL` is available in all supported node versions (introduced in `10.x` if i remember correctly). So we should be able to drop this dependency and use the built-in parser instead. Note that one test here changed hash because url-parse incorrectly did not encode the single quotes in the test URL. With the standard `URL` parsing, it is now encoded.
|
I have added your changes in a new branch to monitor the development. Please check here. Thanks! |
tiwarishubham635
added
type: twilio enhancement
|
no worries, thanks for picking this up! do you want to close this one? since your PR will land the changes anyway |
Yeah, i think we can proceed from this PR now |
| @@ -216,7 +216,7 @@ describe("Request validation", () => { | |||
|
|
|||
| it("should validate urls with special characters", () => { | |||
| const specialRequestUrl = requestUrl + "&Body=It's+amazing"; | |||
| const signature = "dsq4Ehbj6cs+KdTkpF5sSSplOWw="; | |||
| const signature = "TfZzewPq8wqrGlMfyAud8+/IvJ0="; | |||
There was a problem hiding this comment.
I assume this change has been made a test case for checking special characters only, right?
There was a problem hiding this comment.
this changed because URL encodes single quotes, whereas url-parse doesn't
that means the signature is different, but should be ok since url-parse was wrong/off-spec basically
probably is worth you double checking i got the right signature though
the test is still useful i think
There was a problem hiding this comment.
Ahh got it, I checked. The encoding differs in URL than url-parse. This is correct I think, url-parse wasn't encoding quotes and hashtags. The signature is correct, it passes the tests.
|
We can close this PR now, please take a look at #1017 and let us know if everything is good. Thanks! |
We currently use
url-parseto deal with URL construction. Given our node engine constraint of>=14, we knowURLis available in all supported node versions (introduced in10.xif i remember correctly).So we should be able to drop this dependency and use the built-in parser instead.
Note that one test here changed hash because url-parse incorrectly did not encode the single quotes in the test URL. With the standard
URLparsing, it is now encoded.Let me know if there's anything you want me to change here. Also, if someone could confirm i updated the test hash correctly, that'd be very helpful.
Checklist