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.

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

Implementation of URLSearchParams Methods #50043

Closed
wants to merge 9 commits into from

Conversation

riteshshukla04
Copy link
Contributor

@riteshshukla04 riteshshukla04 commented Mar 15, 2025

Summary:

This PR addresses the following issues and enhances the functionality of URLSearchParams:

  1. Extended Initialization Parameters: Previously, only Record<string, string> was supported while initialising URLSearchParams. Now supports string, Record<string, string>, and Array<[string, string]> (aligning with web standards).

  2. Added Implementation for delete(), get(), getAll(), has(), sort(), and set().

  3. Addition of Iteration Methods: Added keys(), values(), entries(), and forEach() methods. Outputs are identical to web.

  4. Bug Fix: Incorrect Initialization from URL. Previously, URLSearchParams was initialized with an empty value when accessed via url.searchParams, even if the URL contained search parameters.

Changelog:

[General][Added] Implementation for URLSearchParams

Test Plan:

Can be tested by below code

  const isHermes = () => !!global.HermesInternal;
  const params = new URLSearchParams("");
  console.log("Is Hermes Enabled:-",isHermes())
  console.log("Values:", Array.from(params.values())); // ✅ ["value1", "value2"]
  console.log("Keys:", Array.from(params.keys())); // ✅ ["key1", "key2"]
  console.log("Entries:", Array.from(params.entries())); // ✅ [["key1", "value1"], ["key2", "value2"]]
  params.forEach((value, key) => {
      console.log(`${key}: ${value}`);
  });
  console.log("Has 'key1'", params.has("key1")); // ✅ true
  console.log("Has 'key3'", params.has("key3")); // ✅ false
  console.log("Get 'key1':", params.get("key1")); // ✅ "value1"
  console.log("Get 'key3':", params.get("key3")); // ✅ null

Tested on both hermes and JSC.

Hermes:-
image

JSC:-
image

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Mar 15, 2025
@riteshshukla04
Copy link
Contributor Author

riteshshukla04 commented Mar 17, 2025

@cortinico Can you please check if this can be merged ? :)

@riteshshukla04
Copy link
Contributor Author

riteshshukla04 commented Mar 24, 2025

@cortinico @cipolleschi Can you please check if we can merge this :)

@cipolleschi
Copy link
Contributor

This PR looks good to me, thanks for working on this.

@cipolleschi
Copy link
Contributor

there are some lint warning though... can you go through them and fix them?

@riteshshukla04
Copy link
Contributor Author

riteshshukla04 commented Mar 24, 2025

Thanks for the review @cipolleschi . I have fixed the warnings . Can you check now

@riteshshukla04
Copy link
Contributor Author

CIs green .🚀

@riteshshukla04
Copy link
Contributor Author

@cipolleschi @cortinico . Can we merge this please before 0.80 starts so that URL and URLSearchParams both are implemented in same version.

@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@huntie huntie requested a review from Copilot March 28, 2025 12:27
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements the full URLSearchParams API, extending its initialization options and adding support for various methods (delete, get, getAll, has, set, keys, values, entries, forEach, sort, toString) to align with web standards.

  • Enhanced initialization of URLSearchParams to support string, record, and array inputs.
  • Added comprehensive tests to validate the new behavior and fixed the URL initialization bug.
  • Updated the URL component in rn-tester to include searchParams output for debugging.

Reviewed Changes

Copilot reviewed 4 out of 6 changed files in this pull request and generated no comments.

File Description
packages/rn-tester/js/examples/Urls/UrlExample.js Added display of URLSearchParams in the URL example.
packages/react-native/Libraries/Blob/tests/URL-test.js Expanded tests for URLSearchParams covering various initialization types and method behaviors.
packages/react-native/Libraries/Blob/URLSearchParams.js Reimplemented URLSearchParams supporting multiple input types and methods in accordance with standards.
packages/react-native/Libraries/Blob/URL.js Fixed URL initialization to properly pass search parameters.
Files not reviewed (2)
  • packages/react-native/Libraries/Blob/URLSearchParams.js.flow: Language not supported
  • packages/react-native/Libraries/tests/snapshots/public-api-test.js.snap: Language not supported
Comments suppressed due to low confidence (3)

packages/react-native/Libraries/Blob/URLSearchParams.js:70

  • The keys method should not accept any parameters as per the URLSearchParams API; please remove the 'name: string' parameter.
keys(name: string): Iterator<string> {

packages/react-native/Libraries/Blob/URLSearchParams.js:74

  • The values method should not define a parameter; remove the 'name: string' argument to conform with the standard API.
values(name: string): Iterator<string> {

packages/react-native/Libraries/Blob/URLSearchParams.js:85

  • The entries method should not accept a parameter; please drop the 'name: string' parameter to align with the web standard.
entries(name: string): Iterator<[string, string]> {

@huntie
Copy link
Member

huntie commented Mar 28, 2025

Note

Just experimenting by adding Copilot as a reviewer on this PR! 😄

Result: Found the 3 API issues under "Comments suppressed due to low confidence (3)"

@riteshshukla04
Copy link
Contributor Author

riteshshukla04 commented Mar 28, 2025

Thanks for the review @huntie and copilot too :) .I have fixed the comments and updated the test cases. Can you check now,

@riteshshukla04 riteshshukla04 requested a review from huntie March 28, 2025 13:34
Copy link
Member

@huntie huntie left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for actioning quickly! :) Just a couple super minor nits.

@riteshshukla04
Copy link
Contributor Author

Nits fixed too :)

@facebook-github-bot
Copy link
Contributor

@huntie has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Mar 29, 2025
@facebook-github-bot
Copy link
Contributor

@huntie merged this pull request in af1f1e4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants