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

feat: move to built-in URL class #998

Closed
wants to merge 3 commits into from
Closed

feat: move to built-in URL class #998

wants to merge 3 commits into from

Conversation

43081j
Copy link

@43081j 43081j commented Jan 27, 2024

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.

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

  • I acknowledge that all my contributions will be made under the project's license
  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the Contribution Guidelines and my PR follows them
  • I have titled the PR appropriately
  • I have updated my branch with the main branch
  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary documentation about the functionality in the appropriate .md file
  • I have added inline documentation to the code I modified

@43081j
feat: move to built-in URL class
c925cb8 
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.
@43081j 43081j mentioned this pull request Jan 27, 2024
Open
@tiwarishubham635
Copy link
Contributor

Hi @43081j! Can you try running make prettier in your local repository before pushing? That should fix the this error.

@tiwarishubham635 tiwarishubham635 mentioned this pull request Apr 5, 2024
Merged
8 tasks
@tiwarishubham635
Copy link
Contributor

I have added your changes in a new branch to monitor the development. Please check here. Thanks!

@tiwarishubham635 tiwarishubham635 added type: twilio enhancement feature request on Twilio's roadmap status: work in progress Twilio or the community is in the process of implementing status: waiting for feedback waiting for feedback from the submitter difficulty: medium fix is medium in difficulty and removed type: twilio enhancement feature request on Twilio's roadmap status: work in progress Twilio or the community is in the process of implementing labels Apr 5, 2024
@43081j
Copy link
Author

43081j commented Apr 5, 2024

no worries, thanks for picking this up!

do you want to close this one? since your PR will land the changes anyway

@tiwarishubham635
Copy link
Contributor

tiwarishubham635 commented Apr 5, 2024 •
edited

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

tiwarishubham635
tiwarishubham635 reviewed Apr 5, 2024
View reviewed changes
spec/validation.spec.js
@@ -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=";
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this change has been made a test case for checking special characters only, right?

Copy link
Author

@43081j 43081j Apr 5, 2024 •
edited

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@tiwarishubham635
Copy link
Contributor

We can close this PR now, please take a look at #1017 and let us know if everything is good. Thanks!

@43081j 43081j closed this Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tiwarishubham635 tiwarishubham635 tiwarishubham635 left review comments

Assignees
No one assigned
Labels
difficulty: medium fix is medium in difficulty status: waiting for feedback waiting for feedback from the submitter
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@43081j @tiwarishubham635