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

Implement URL Accessors (host, hostname, username, password) for the URL class #48505

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

krishpranav
Copy link

#45030

Title:
Implement URL Accessors (host, hostname, username, password) for the URL class

Description:

Summary:

This PR addresses GitHub issue #45030 by implementing the missing URL accessors (host, hostname, username, and password) in the URL class for React Native. Prior to this change, accessing these properties would result in errors as they were not implemented. This update ensures compatibility with standard URL parsing and usage within the React Native ecosystem, especially for applications relying on web-based APIs that require access to these URL components.

Motivation:

These accessors are critical for extracting and working with specific URL components (e.g., domain, user credentials) in various networking scenarios. The lack of implementation was preventing the proper usage of these components in React Native, particularly when interacting with URLs that include authentication information or need parsing of domain-related properties.

Changelog:

  • [INTERNAL] [ADDED]: Implemented host, hostname, username, and password accessors for the URL class in React Native.
  • [INTERNAL] [CHANGED]: Updated the URL class to support retrieval of host, hostname, username, and password from the URL string, ensuring these properties return the correct values based on the URL structure.

Test Plan:

To ensure the functionality of the newly implemented accessors, I performed the following:

  1. Unit Tests:

    • Ran the existing unit tests, which pass successfully.
    • Added tests to check for correct implementation of host, hostname, username, and password accessors in various scenarios.
  2. Test Cases:

    • Validated various URL formats to ensure that all accessors return the expected values.
    • Verified that the new accessors handle both standard and edge-case URLs, including those with and without authentication information.

Here is a sample test case:

const testUrl = new URL('https://username:[email protected]:8080/path');
expect(testUrl.host).toBe('domain.com:8080');
expect(testUrl.hostname).toBe('domain.com');
expect(testUrl.username).toBe('username');
expect(testUrl.password).toBe('password');

All tests passed successfully, demonstrating that the changes are robust and do not introduce regressions.


Screenshot 2025-01-06 at 6 47 01 PM

Kindly let me know whether is there any further things needed to add on this pull request, I am a new contributor to this repo. I am looking forward to work on this repo and work on the untouched issues which needed fix. thanks :)

  • Krisna Pranav

@facebook-github-bot
Copy link
Contributor

Hi @krishpranav!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@krishpranav
Copy link
Author

I've signed the cla

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@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 Jan 6, 2025
@krishpranav
Copy link
Author

Hi, any updates on it?

@javache
Copy link
Member

javache commented Jan 7, 2025

This is a very commonly used class - I don't think we'd want to pay the cost upfront of parsing any URL passed in here.

Comment on lines 22 to 23
// $FlowFixMe[incompatible-type] asserted above
// $FlowFixMe[unsafe-addition]
Copy link
Member

Choose a reason for hiding this comment

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

Why was this removed?


function validateBaseUrl(url: string) {
// from this MIT-licensed gist: https://gist.github.com/dperini/729294
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this

Comment on lines +101 to +102
// Parsing the URL to use for accessors
this._parsedUrl = new globalThis.URL(this._url);
Copy link
Member

Choose a reason for hiding this comment

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

Which JSVM was this tested with?

Copy link
Author

Choose a reason for hiding this comment

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

node version: v20.18.1

@krishpranav
Copy link
Author

@javache reverting the comments.

@krishpranav
Copy link
Author

@javache I've reverted the comments. kindly check it.

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. 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.

3 participants