Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(node): fully implement node:querystring and node:url #299

Merged
merged 3 commits into from
Sep 4, 2024
Merged

Conversation

pi0
Copy link
Member

@pi0 pi0 commented Aug 29, 2024

fixes #290
alternative of #292

Ported implementation of node:url and node:querystring from Node.js v22.7.0.

Remarks:

  • Process was semi-automated and I tried to not touch implementation as much as possible
  • Type checks were also added and passed, although some type safety (mostly for nullable values) had been bypassed trusting upstream implementation is safe
  • Ported tests/parallel/test-{querystring,url}* tests which pass as expected
    • Invalid IDNA seems to be uncaught. possibly related to punycode behavior not matching native format binding
    • Whatwg URL format with unicode+auth options fails

@pi0 pi0 self-assigned this Aug 29, 2024
@IgorMinar IgorMinar self-requested a review August 29, 2024 16:45
@pi0 pi0 requested a review from anonrig August 29, 2024 17:49
Co-authored-by: Yagiz Nizipli <yagiz@nizipli.com>
@pi0
Copy link
Member Author

pi0 commented Sep 2, 2024

dear @IgorMinar i noticed you added yourself for review. Do you have any specific concerns about this PR?

I think for next step of adopting hybrid support for workerd, we can try with another PR. (and i have two different methods i like to discuss with your team how we can do it).

@pi0 pi0 marked this pull request as ready for review September 4, 2024 14:12
@pi0
Copy link
Member Author

pi0 commented Sep 4, 2024

(merging to test against nightly channel and iterate)

@pi0 pi0 merged commit 941309d into main Sep 4, 2024
2 checks passed
@pi0 pi0 deleted the feat/qs-url branch September 4, 2024 14:13
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

url.parse('/404') throws, unlike in Node.js
2 participants