Fixing the support of WHATWG URLs #28

sadasant · 2019-07-06T18:33:00Z

This is an attempt to support WHATWG URLs.
Tests are passing 💚
Feedback is appreciated!

Note: I'm keeping the internal options as a plain object in index.js, but this shouldn't affect the rest of the behavior we're expecting.

Fixes #26

This is an attempt to support WHATWG URLs. Feedback is appreciated! Fixes #26

BYK

On top of the review comments, we now need new test cases to make sure the new support actually works and won't break in the future.

BYK · 2019-07-08T15:00:08Z

index.js

+function urlInstanceToObject(url) {
+  var urlObject = {};
+  // If the opts are genearted with the WHATWG API
+  if (url && Object.getOwnPropertySymbols && Object.getOwnPropertySymbols(url)[0]) {


Some more comments or pointers explaining this magic would be very useful for the future reader. Even now I don't fully understand what is happening :D

I'll add comments for sure! First here, since it's easier:

This line is about checking whether the url is a WHATWG url or not. I'm doing this by first checking that the url exists (is not falsy), then checking that Object.getOwnPropertySymbols exists (since this feature appeared relatively recently, compared the versions of Node that are expected to work on CI), and finally by combining this Object method with the given URL to confirm that it indeed has symbols.

I currently don't know a better way of doing this! I pushed something fast that seemed to work. I'm interested in learning how to make this better while updating this as fast as I can! 🌻

What do you think of this approach? Any suggestion?

BYK · 2019-07-08T15:01:08Z

index.js

+      urlObject[key] = urlContext[key];
+    }
+  } else {
+    urlObject = Object.assign({}, url);


@TooTallNate - are you okay dropping support for Node < 8 which would allow us to use object spread and maybe some more niceties?

That is a great question!

I'd like to keep Node 6 support at least.

BYK · 2019-07-08T15:01:40Z

index.js

@@ -28,7 +44,7 @@ function Agent(callback, _opts) {
  // (i.e. it has a callback function) lazily
  this._promisifiedCallback = false;

-  let opts = _opts;
+  var opts = _opts;


let inside blocks breaks some of the environments that are supposed to work on CI (here and in this other PR: TooTallNate/proxy-agents#71 ). I assumed that making all vars would just help us keep consistency! I can change it back for sure, just let me know.

index.js

BYK · 2019-07-08T15:07:22Z

index.js

@@ -91,8 +107,8 @@ Agent.prototype.addRequest = function addRequest(req, _opts) {
  req.shouldKeepAlive = false;

  // Create the `stream.Duplex` instance
-  let timeout;
-  let timedOut = false;
+  var timeout;


Please no vars :)

On CI we're testing Node 4 and 5, which doesn't have a proper functional let, I believe. If we drop support of Node < 6 I would agree to stop using vars!

BYK · 2019-07-08T15:08:43Z

patch-core.js

+        _url = undefined;
+      }
+      if (!_url && _options) {
+        _url = url.format(_options);


Why do we do this? Which case are we handling here?

https.request has two valid behaviors:

https.request(options[, callback]) https.request(url[, options][, callback])

For that reason, if _url isn't a string and there's no callback, between line 19 and 21 we're assuming the scenario with only two parameters was used, so we're re-assigning the parameters to make sense with their correspondent name. Right at the end of that, I'm assigning undefined to _url, trying to simplify what we're trying to accomplish in that if block.

After that, I'm only checking that if we don't have a _url and we do have _options, we can now try to transform the _options to have a string _url. We do this by using url.format, which returns a string and works for both legacy URLs and WHATWG URLs. This second if block also helps if the user decided to pass a falsy _url, since any form of a falsy _url will break this anyway.

Feedback appreciated! 💚

BYK · 2019-07-08T15:10:12Z

patch-core.js

      if (typeof _options === 'string') {
        options = url.parse(_options);
      } else {
-        options = Object.assign({}, _options);
+        options = url.parse(_url);


This case feels unsafe as we simply discard the original value of _options. What is the assumption we are making here that gives us the ability to discard the old value?

At this point we're sure to have a _url. What we're doing next is to grab the options out of the _url and merge them with the received options. In https.get we had this line options = Object.assign({}, url.parse(_url), _options) that had the same intentions of merging both _url and options into one object. What I'm doing is to make it work with the new WHATWG structure, as well as to make sure that both https.get and https.request follow this behavior.

BYK · 2019-07-08T15:10:44Z

patch-core.js

-        options = Object.assign({}, _options);
+        options = url.parse(_url);
+        if (_options) {
+          if (_options && Object.getOwnPropertySymbols && Object.getOwnPropertySymbols(_options)[0]) {


This code looks repeated, consider abstracting out into a util function?

Sounds good!

I did abstract this into a re-usable function! Please check the update!

BYK · 2019-07-08T15:12:12Z

patch-core.js

+  } else {
+    options = url.parse(_url);
+    if (_options) {
+      if (_options && Object.getOwnPropertySymbols && Object.getOwnPropertySymbols(_options)[0]) {


Again, this logic now needs more abstraction and explaining :)

I did abstract this into a re-usable function! Please check the update!

kamilogorek · 2019-09-13T08:13:14Z

Ping @TooTallNate

kunal15595 · 2019-10-16T04:44:13Z

Any progress here?

TooTallNate · 2019-12-06T00:56:54Z

Sorry for letting this one get stale. The project has been refactored to TypeScript and this PR needs to be refactored at this point as well.

Fixing the support of WHATWG URLs

ed48b77

This is an attempt to support WHATWG URLs. Feedback is appreciated! Fixes #26

sadasant requested review from TooTallNate and BYK July 6, 2019 18:33

sadasant self-assigned this Jul 6, 2019

This was referenced Jul 6, 2019

HTTPS.Request() does not accept WHATWG URL #26

Closed

WIP: Fixing the support of WHATWG URLs TooTallNate/proxy-agents#71

Closed

fixing the changes for older versions of Node

34c7e84

sadasant force-pushed the feature/fix26 branch from db8acc1 to 34c7e84 Compare July 6, 2019 18:45

BYK suggested changes Jul 8, 2019

View reviewed changes

addressed some of the feedback

22faf21

kamilogorek mentioned this pull request Jul 10, 2019

https.request is broken by agent-base included by https-proxy-agent getsentry/sentry-javascript#2085

Closed

1 task

TooTallNate closed this Jan 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing the support of WHATWG URLs #28

Fixing the support of WHATWG URLs #28

sadasant commented Jul 6, 2019 •

edited

Loading

BYK left a comment

BYK Jul 8, 2019

sadasant Jul 8, 2019 •

edited

Loading

BYK Jul 8, 2019

sadasant Jul 8, 2019

TooTallNate Oct 4, 2019

BYK Jul 8, 2019

sadasant Jul 8, 2019

BYK Jul 8, 2019

sadasant Jul 8, 2019

BYK Jul 8, 2019

sadasant Jul 8, 2019

BYK Jul 8, 2019

sadasant Jul 8, 2019

BYK Jul 8, 2019

sadasant Jul 8, 2019

sadasant Jul 9, 2019

BYK Jul 8, 2019

sadasant Jul 9, 2019

kamilogorek commented Sep 13, 2019

kunal15595 commented Oct 16, 2019

TooTallNate commented Dec 6, 2019

Fixing the support of WHATWG URLs #28

Fixing the support of WHATWG URLs #28

Conversation

sadasant commented Jul 6, 2019 • edited Loading

BYK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasant Jul 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamilogorek commented Sep 13, 2019

kunal15595 commented Oct 16, 2019

TooTallNate commented Dec 6, 2019

sadasant commented Jul 6, 2019 •

edited

Loading

sadasant Jul 8, 2019 •

edited

Loading