Skip to content
This repository has been archived by the owner on Jul 12, 2019. It is now read-only.

Fix TypeError on requestUrl.indexOf #59

Merged
merged 2 commits into from
Dec 20, 2017
Merged

Fix TypeError on requestUrl.indexOf #59

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b09408f
Fix TypeError on requestUrl.indexOf
eliangcs Dec 19, 2017
e754267
Use template instead of string addition
eliangcs Dec 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions src/tools/create-http-patch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,20 @@ const createHttpPatch = event => {

// Proxy the request method
object.request = (options, callback) => {
const requestUrl =
options.url ||
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this isn't re-added anywhere, right? So this will break existing code that is passing in objects with a .url property, which may not be intended.

Copy link
Member Author

@eliangcs eliangcs Dec 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

If I understand the code correctly, object.request is to patch Node's http.request method, which does not accept an object with a .url property:

> let http = require('http');
undefined
> http.request({url:'http://httpbin.org/get'});
...
> Error: connect ECONNREFUSED 127.0.0.1:80
    at Object.exports._errnoException (util.js:1018:11)
    at exports._exceptionWithHostPort (util.js:1041:20)
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1090:14)

If the point is to follow http.request's spec 100%, then we should not accept an object with a .url property here neither. But I'm not sure if any existing code or library is doing that (passing an object with a .url property). Any clues about that?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, fair deal! should be safe to go then.

options.href ||
`${options.protocol || 'https://'}${options.host}${options.path}`;
// `options` can be an object or a string. If options is a string, it is
// automatically parsed with url.parse().
// See https://nodejs.org/docs/latest-v6.x/api/http.html#http_http_request_options_callback
let requestUrl;
if (typeof options === 'string') {
requestUrl = options;
} else {
requestUrl =
options.href ||
`${(options.protocol || 'https:') + '//'}${options.host}${
Copy link
Contributor

Choose a reason for hiding this comment

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

super minor, but you can have the // in the template string itself instead of doing the addition operation:

${(options.protocol || 'https:')}//${options.host}...

Copy link
Member Author

@eliangcs eliangcs Dec 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Good suggestion. Fixed in e754267.

options.path
}`;
}

const logger_url =
process.env.LOGGING_ENDPOINT || constants.DEFAULT_LOGGING_HTTP_ENDPOINT;

Expand Down