Skip to content

feat: implement node querystring #1528

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
Merged
sgammon merged 14 commits into from
Jul 18, 2025
Merged

feat: implement node querystring #1528

sgammon merged 14 commits into from
Jul 18, 2025

Conversation

@franklinfollis
Copy link
Collaborator

@franklinfollis franklinfollis commented Jul 10, 2025 •
edited
Loading

Draft Powered by Pull Request Badge

Summary

First pass at implementing Node's querystring module.

@franklinfollis franklinfollis self-assigned this Jul 10, 2025
@franklinfollis franklinfollis added feature Large PRs or issues with full-blown features 🚧 WIP Works-in-progress. Blocks merge lang:javascript Issues relating to JavaScript api:node Node API and stdlib labels Jul 10, 2025
@codecov
Copy link

codecov bot commented Jul 10, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 71.19342% with 70 lines in your changes missing coverage. Please review.

Project coverage is 40.88%. Comparing base (ea278e3) to head (56ef2fc).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
.../elide/runtime/node/querystring/NodeQuerystring.kt 77.33% 6 Missing and 28 partials ⚠️
...time/intrinsics/js/node/querystring/QueryParams.kt 56.00% 17 Missing and 5 partials ⚠️
...trinsics/js/node/querystring/QuerystringOptions.kt 48.14% 12 Missing and 2 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1528      +/-   ##
==========================================
+ Coverage   40.67%   40.88%   +0.20%     
==========================================
  Files         749      752       +3     
  Lines       35332    35571     +239     
  Branches     4968     5031      +63     
==========================================
+ Hits        14372    14542     +170     
- Misses      19246    19280      +34     
- Partials     1714     1749      +35
Flag Coverage Δ
jvm 40.88% <71.19%> (+0.20%) ⬆️
lib 40.88% <71.19%> (+0.20%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...elide/runtime/intrinsics/js/node/QuerystringAPI.kt 100.00% <100.00%> (ø)
...trinsics/js/node/querystring/QuerystringOptions.kt 48.14% <48.14%> (ø)
...time/intrinsics/js/node/querystring/QueryParams.kt 56.00% <56.00%> (ø)
.../elide/runtime/node/querystring/NodeQuerystring.kt 77.92% <77.33%> (-9.58%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea278e3...56ef2fc. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sgammon sgammon added this to the Release R18: Beta milestone Jul 13, 2025
@franklinfollis franklinfollis removed the 🚧 WIP Works-in-progress. Blocks merge label Jul 17, 2025
@franklinfollis franklinfollis marked this pull request as ready for review July 17, 2025 12:33
@franklinfollis franklinfollis requested a review from sgammon as a code owner July 17, 2025 12:33
darvld
darvld previously requested changes Jul 17, 2025
View reviewed changes
Copy link
Member

@darvld darvld left a comment

Choose a reason for hiding this comment

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

Great work overall, just some nits

@franklinfollis franklinfollis force-pushed the feat/node-querystring branch from 8b6eab6 to 56ef2fc Compare July 18, 2025 06:09
@franklinfollis franklinfollis requested a review from darvld July 18, 2025 06:40
@sgammon sgammon merged commit 13bc39d into main Jul 18, 2025
18 checks passed
@elidebot elidebot mentioned this pull request Jul 19, 2025
Merged
29 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sgammon sgammon sgammon approved these changes

@darvld darvld Awaiting requested review from darvld

Assignees

@franklinfollis franklinfollis

Labels

api:node Node API and stdlib feature Large PRs or issues with full-blown features lang:javascript Issues relating to JavaScript

Projects

None yet

Milestone

Release R18: Beta

Development

Successfully merging this pull request may close these issues.

4 participants

@franklinfollis @sgammon @darvld