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

WHATWG-URL support #228

Closed
stevenvachon opened this issue Oct 16, 2016 · 22 comments
Closed

WHATWG-URL support #228

stevenvachon opened this issue Oct 16, 2016 · 22 comments
Labels
enhancement This change will extend Got features ✭ help wanted ✭

Comments

@stevenvachon
Copy link
Contributor

URL / whatwg-url is the future.

@sindresorhus
Copy link
Owner

sindresorhus commented Oct 17, 2016

Please include a more comprehensive issue description.

What exactly would you like to see changed in Got?

@stevenvachon
Copy link
Contributor Author

This library does not support URL as it tries to merge parsed URL objects into other objects.

@sindresorhus
Copy link
Owner

Can you provide a code example of how you would like to use URL with Got?

@stevenvachon
Copy link
Contributor Author

stevenvachon commented Oct 24, 2016

const url = new URL("http://domain/")
got(url).then()

@sindresorhus sindresorhus added enhancement This change will extend Got features ✭ help wanted ✭ labels Feb 5, 2017
@alextes
Copy link
Contributor

alextes commented Feb 13, 2017

I'd love to help out with this one. Let's see if we can sketch a clear picture of the problem that needs solving.

Got currently expects an URL to be a simple string (which it parses), or a node http request options object. In the future, node devs will ideally rely on WHATWG-URL.

  • got should correctly 'normalize' URL. So recognize and behave.
    • there's some object assigning going on which is incompatible with the way URL works.

I'll also do a naive implementation and see what breaks. Let me know if there's anything I missed and should consider before opening a PR!

/cc @stevenvachon since you are clearly knowledgeable in the matter and might feel like giving me some pointers 😄.

@stevenvachon
Copy link
Contributor Author

stevenvachon commented Feb 13, 2017

This may not be necessary, as http.request and https.request support URL natively. fs functions will soon.

@alextes
Copy link
Contributor

alextes commented Feb 13, 2017

I see. The URL object looks just like the options object http has expected since forever (forgive me not digging around to find out how far back this works). So that would just leave not manipulating the URL object in a way that breaks it. I'll look into it! Thanks @stevenvachon!

@stevenvachon
Copy link
Contributor Author

stevenvachon commented Feb 13, 2017

Such support has been implemented in v7.x and, like all other URL features, will continue to be experimental and undocumented until v8.0

The main difference URL brings to properties on the request config object is a non-existent auth key, instead having username and password.

@alextes
Copy link
Contributor

alextes commented Feb 13, 2017

Alright, I played around with it a little. If I understand you correctly. Got would ideally be usingURL for its internal representation of URLs. Until got can support only Node v8 and up we can't rely on node for that, but we can use whatwg-url.

This means:

  • no more object.assigning the props of the url object because that means we lose our pretty URL structure.
  • some implications for dealing with auth I'll have to dig into.

Sounds like I can get to work 😄.

@alextes
Copy link
Contributor

alextes commented Mar 7, 2017

Started work on this, after a bunch of contemplating I realised I probably misunderstood something.

@stevenvachon could you please verify all you meant earlier was for got to stop breaking whatwg-url on Node v8+ to seamlessly start supporting them whenever Node does?

I'm quite sure it's what you meant. I'll continue work assuming this is true 😄. Sorry for taking so long to understand what you were after. In case you also meant for got to start supporting whatwg-url on Node v4+ (or in a month probably v6+) please give a suggestion as to how, or ask me to share some options I had puzzled out.

@stevenvachon
Copy link
Contributor Author

stevenvachon commented Mar 7, 2017

I've been writing universal-url for seamless Node v4+ and browser support.

@sindresorhus
Copy link
Owner

I don't think it's essential enough to add more dependencies for Node.js 4 support, but happy to add support for WHATWG URL when a user is on Node.js 8.

@stevenvachon
Copy link
Contributor Author

stevenvachon commented Mar 7, 2017

Maybe I'll write a "has-whatwg-url" or similar package. Though, since this will likely be input only, isurl may only be necessary.

@alextes
Copy link
Contributor

alextes commented Mar 7, 2017

I've seen isomorphic-url! Nice to see you're building libs to help out the ecosystem.

I agree with sindre that it seems a bit much to start supporting whatwg-url in node v4+ for such a lightweight lib.

I would be happy to add v8+ support for whatwg-url too! (pls don't take 'my' PR @sindresorhus 😢). The only thing I'm not sure about is given that I find a way to make passing the `whatwg-url' in v8, and options object otherwise work, how does one feature detect node whatwg-url support or the running node version to be v8+?

I guess for now I'm moving on to another issue, thanks you two 😄.

@stevenvachon
Copy link
Contributor Author

@alextes for what reason do you need URL feature detection? Could you not just check if the input is a URL and perform different operations (that are build into its instance)?

@alextes
Copy link
Contributor

alextes commented Mar 8, 2017

Sorry, that's what I meant @stevenvachon. I confused terms.

@alextes
Copy link
Contributor

alextes commented Mar 8, 2017

Still lost on what you guys would like to see implementation wise. I can imagine:

  1. not assigning parsed url opts because that breaks for whatwg-url objects. Instead I could leave the object intact and just read bits from it or passed 'got opts' as appropriate. Meaning it would JustWork™ on Node 8.
  2. detect whatwg-url and turn it into something that works with the current approach, leaving the rest of the code unchanged.

@stevenvachon
Copy link
Contributor Author

stevenvachon commented Mar 8, 2017

You could do both. #­1 would require a new module ("hasurl") that I could publish with code pulled from isomorphic-url. #­2 would require isurl for situations where hasURL === false.

@stevenvachon
Copy link
Contributor Author

stevenvachon commented Mar 9, 2017

@alextes check out hasurl for feature testing URL support. Currently, only Node 8.x nightlies will return true.

@alextes
Copy link
Contributor

alextes commented Mar 9, 2017

@stevenvachon 🙌
I'll see about taking a stab at this.

@alextes
Copy link
Contributor

alextes commented Mar 26, 2017

@stevenvachon please check out the PR. You can probably explain why it's not okay to serialize and parse whatwg-url. I have the feeling I'm still not fully grasping your / whatwg's intentions here.

@stevenvachon
Copy link
Contributor Author

stevenvachon commented Apr 3, 2017

My original intent was to drop the use of url.parse for URL, which would require a re-implementation of "url-parse-lax". Keeping url.parse does not reach the goal of decreasing browserify bundle size (when/if paired with universal-url-lite), but it does avoid a major version bump.

Regarding your PR, I don't see why a re-parse is necessary when you can abstract the mismatching properties with variables:

let auth;
if (!isURL(url)) {
  url = urlParseLax(url);
  auth = url.auth;
}
else {
  auth = `${url.username}:${url.password}`;
  if (auth === ':') auth = '';
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This change will extend Got features ✭ help wanted ✭
Projects
None yet
Development

No branches or pull requests

3 participants