Added haxe.Http support for nodejs #4713
Conversation
|
@nadako can you check + merge ? |
|
I haven't used |
|
We don't have tests for |
|
Not really, last time I tried to set that up it ended in tears. |
|
@Blank101 It's missing |
|
Oops missed the onStatus, and yes requestUrl, withCredentials and async should be unavailable. |
|
Another thing that's missing is setting multiple headers of the same name. It seems that it's also missing in |
| protocol: secure ? "https:" : "http:", | ||
| host: host, | ||
| port: port, | ||
| method: post == true ? 'POST' : 'GET', |
There was a problem hiding this comment.
post could be null, no?
There was a problem hiding this comment.
lol I guess that doesn't make a difference for js target..
|
added some more comments. to be honest, I think we should refactor |
|
Made the change, and yes I agree about refactoring haxe.Http. It's a mess right now. |
|
I would also prefer to see this thing refactored before we add any more stuff to it... |
| #if js | ||
| #if nodejs | ||
| me.responseData = null; | ||
| var url_regexp = ~/^(https?:\/\/)?([a-zA-Z\.0-9_-]+)(:[0-9]+)?(.*)$/; |
There was a problem hiding this comment.
Wouldn't it be easier to use js.node.Url.parse instead of own solution?
There was a problem hiding this comment.
Would probably make it easier to maintain. I just copied the regex from the customRequest.
|
Okay, about multiple headers with same name, it seems that node.js supports passing array of string instead of string for the header value. I'll update the hxnodejs extern, meanwhile please add support for that in |
|
The resource you posted is for http response headers, not request. |
|
Yes, but actually |
|
And those methods are used to set headers passed as |
…odejs Url.parse()
|
Are we able to merge this in for the time being, and defer the refactor to some point in the future? This is a pretty critical feature for nodejs users. |
|
I don't know, this introduces dependency on hxnodejs in the std code and I'm not sure this is a good idea. I'd rather go for HaxeFoundation/hxnodejs#69 |
|
This can't go into hxnodejs because of HaxeFoundation/hxnodejs#42, so I just merged this at last. |
Can you elaborate? I offered a solution for having a node-specific Bytes implementation but I fail to see how it's related to Http. I really think we should either add nodejs to the std lib or put http for nodejs into hxnodejs. This kind of mix is pretty horrible. |
If I just redefine haxe.Http in hxnodejs it will break usage of haxe.Http macros. I tried adding it to I'm yet to try the solution you suggested here HaxeFoundation/hxnodejs#42 (comment). It sounds like it might actually work, but still feels hacky to me (still an option tho).
Well we used to depend on hxssl for the https support, this isn't very different. |
I'm using this technique quite a lot. Maybe it's a hack (I guess overwriting a std class probably always will be). But it is isolated and allows for clean separation on the scale that really matters. Once better features are available, it is trivial to replace the hack.
Yes and that was horrible. I agree this isn't very different, particularly in that this is horrible too :D |
No description provided.