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

Slight URL parsing update #663

Closed
DoctorMalice opened this issue Nov 21, 2022 · 2 comments
Closed

Slight URL parsing update #663

DoctorMalice opened this issue Nov 21, 2022 · 2 comments
Labels
enhancement New feature or request

Comments

@DoctorMalice
Copy link

I would like to be able to set the window.location.href value to a value that uses any protocol and have it returned as expected. Currently I have a test like the following that fails:

window.location.href = 'git://test'
expect(window.location.href).toEqual('git://test')

The actual value is //git://test

I believe the reason is because the url parser only understands http and https as protocols (given the regex in the URL.ts file):

const URL_REGEXP = /(https?:)\/\/([-a-zA-Z0-9@:%._\+~#=]{2,256}[a-z]{2,6})(:[0-9]*)?([-a-zA-Z0-9@:%_\+.~c&//=]*)(\?[^#]*)?(#.*)?/;

So when the url is retrieved, '//' is prepended due to there being no protocol in this code:

public get href(): string {
  const credentials = this.username ? `${this.username}:${this.password}@` : '';
  return this.protocol + '//' + credentials + this.host + this.pathname + this.search + this.hash;
}

I'm not a big picture kind of person, so I may not be thinking of the issues associated with the following change, but the url (and any url with a custom protocol) is parsed properly with the following regex:

const URL_REGEXP = /(.+:)\/\/([-a-zA-Z0-9@:%._\+~#=]{2,256}[a-z]{2,6})(:[0-9]*)?([-a-zA-Z0-9@:%_\+.~c&//=]*)(\?[^#]*)?(#.*)?/;

An alternative solution may be only prepending the '//' when protocol is defined, but that may open up issues with protocol relative URLs.

@DoctorMalice DoctorMalice added the enhancement New feature or request label Nov 21, 2022
@Mas0nShi
Copy link
Contributor

Duplicate of #401 #521 #569

Hi, @DoctorMalice, It's has been solved in #520. (:
but this is a complex PR and therefore probably won't merge upstream anytime soon.

@capricorn86
Copy link
Owner

Thanks for reporting @DoctorMalice! 🙂

We have now fixed this by using the native url module.

You can read more about the release here:
https://github.com/capricorn86/happy-dom/releases/tag/v7.8.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants