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

src: add node::url::URL::href() method #22610

Closed
alexkozy opened this issue Aug 31, 2018 · 19 comments
Closed

src: add node::url::URL::href() method #22610

alexkozy opened this issue Aug 31, 2018 · 19 comments
Labels
feature request Issues that request new features to be added to Node.js. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. whatwg-url Issues and PRs related to the WHATWG URL implementation.

Comments

@alexkozy
Copy link
Member

Is your feature request related to a problem? Please describe.
I need to get href based on instance of URL object in native code.

Describe the solution you'd like
I can use href() method that is added in addition to existing getters.

@alexkozy alexkozy added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 31, 2018
@addaleax addaleax added whatwg-url Issues and PRs related to the WHATWG URL implementation. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Sep 3, 2018
@addaleax
Copy link
Member

addaleax commented Sep 3, 2018

@nodejs/url

@devsnek
Copy link
Member

devsnek commented Sep 3, 2018

the url serializer is written in js, so i'm not 100% sure if this is possible without a lot of code duplication

@TimothyGu
Copy link
Member

@devsnek Yeah my guess is that we will have to duplicate. That's fine though, as we already have a duplicated URL class in C++ anyway.

@tigercosmos
Copy link

tigercosmos commented Sep 15, 2018

Are there any specs for URL::href()?

@tigercosmos
Copy link

I am not sure if is this one
https://url.spec.whatwg.org/#dom-url-href

@TimothyGu
Copy link
Member

@tigercosmos Yes! You can even look at how lib/internal/url.js implements it in JavaScript, and just duplicate that logic in C++.

@WaleedAshraf
Copy link
Contributor

Hi @ak239
I think you can use toString() also?
They both return the same result.

@stropitek
Copy link
Contributor

I'm taking over this issue

@addaleax
Copy link
Member

@stropitek As @WaleedAshraf commented, this might not be necessary since ToString() also works… just as a heads up, so you don’t do unnecessary work :)

@davesters
Copy link
Contributor

May be worth it to dig deeper to verify if ToString() actually exists and works in C++.

If it does to remove this TODO and update to use the ToString() function.
https://github.com/ak239/node/blob/master/src/inspector_agent.cc#L622

@stropitek
Copy link
Contributor

No the ToString method exists on URLHost class not on the URL class.

@Trott
Copy link
Member

Trott commented Nov 13, 2018

Is this being actively worked on?

@Trott Trott added the feature request Issues that request new features to be added to Node.js. label Nov 18, 2018
@Trott
Copy link
Member

Trott commented Nov 18, 2018

@ak239 This is still something you want/need? Or did you find some other workaround?

@WaleedAshraf
Copy link
Contributor

@Trott
@addaleax and I looked into this on Code + Learn. ToString() method can serve the purpose. I think this can be closed.

@targos
Copy link
Member

targos commented Nov 18, 2018

There is no URL::ToString method and AFAIK @stropitek has a wip branch to add it

@WaleedAshraf
Copy link
Contributor

Yup. That's URLHost not URL. We were confused.

@Trott
Copy link
Member

Trott commented Nov 24, 2018

There is no URL::ToString method and AFAIK @stropitek has a wip branch to add it

Would it make sense to open a WIP PR?

@Trott
Copy link
Member

Trott commented Apr 22, 2020

This seems like it stalled. I don't know if @ak239 still needs this after all this time and I don't know that anyone else is asking for it. I'd be inclined to close it unless @ak239 or someone else indicates it is still a desirable feature for them and/or someone opens a PR.

@alexkozy
Copy link
Member Author

As far as I remember the motivation behind this one was driven by one of the comments in the code review for one of my PRs: #22251 (comment)
It looks like there were no other requests for href getter over the last year and a half so I will close this one. Feel free to reopen it if you think that we need href getter.

watilde added a commit that referenced this issue Nov 21, 2020
PR-URL: #35912
Refs: #22610
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this issue Nov 22, 2020
PR-URL: #35912
Refs: #22610
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

No branches or pull requests

10 participants