-
Notifications
You must be signed in to change notification settings - Fork 15
Conversation
I love Zapier - great product. Would you be interested in software that automatically finds TypeErrors like this? I am building Fuzz Stati0n to do that (free for OSS) - please take a look and consider signing up for our newsletter to keep in touch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the .url
thing is a breaking change, we should fix it. if not, you're probably good to go. the string thing is super not worth its own commit.
@@ -18,10 +18,20 @@ const createHttpPatch = event => { | |||
|
|||
// Proxy the request method | |||
object.request = (options, callback) => { | |||
const requestUrl = | |||
options.url || |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
src/tools/create-http-patch.js
Outdated
} else { | ||
requestUrl = | ||
options.href || | ||
`${(options.protocol || 'https:') + '//'}${options.host}${ |
There was a problem hiding this comment.
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}...
There was a problem hiding this comment.
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.
Encountered the following error when I was working on zapier/zapier-platform-cli#208.
Turned out that
options.url
is an object as opposed to a string in this failed test case. According to http.request's doc,options
is either a string (URL) or an object (request options). It doesn't say anything aboutoptions.url
. I'm not sure why the previous implementation didoptions.url || options.href
, but here we go.