search

cometd / cometd-nodejs-client

CometD client for NodeJS
Apache License 2.0
17 stars 7 forks source link

Various issues #5

Closed ccoenraets closed 7 years ago

ccoenraets commented 7 years ago

Am I using this right or is there another way to use this module? Thanks

sbordet commented 7 years ago

The module does have an adapt() function (https://github.com/cometd/cometd-nodejs-client/blob/master/cometd-nodejs-client.js#L2) and you must call it to have the module work properly.

ccoenraets commented 7 years ago

The version installed via npm (1.0.0-BETA0) does not have an adapt() function. Will try with the version from the repo. Thanks.

sbordet commented 7 years ago

Install latest version on tag beta.

ccoenraets commented 7 years ago

OK, got 1.0.0-BETA3 with adapt() function. I'm still getting ReferenceError: XMLHttpRequest is not defined. I'm patching it with global.XMLHttpRequest = window.XMLHttpRequest; in my module, but then getting Error: Protocol "https:" not supported. Expected "http:". Thanks.

sbordet commented 7 years ago

Install cometd at tag beta as well. Sorry, stuff is still experimental :)

ccoenraets commented 7 years ago

No problem. Appreciate the hard work on the library. The beta of cometd fixed the first problem (XMLHttpRequest is not defined), but it looks like the adapter does not support https. Still getting Protocol "https:" not supported. Expected "http:".

ccoenraets commented 7 years ago

Quick fix to make it work in my specific use case. require('https'), and add Content-Length header: _config.headers['Content-Length'] = JSON.stringify(data).length;.

sbordet commented 7 years ago

Can you please detail how you get the "https:" not supported. Expected "http:" error ?

sbordet commented 7 years ago

I think I understand the https error now... I guess you're using https: URLs, but in the code I only use the http module to send requests, while I should switch using either the http module or the https module depending on the URL scheme.

The suggestion you made about the Content-Length header is not correct since the length of the string is not equal to the length in bytes in case of non-ASCII characters.

sbordet commented 7 years ago

@ccoenraets I filed #6 to isolate the https issue. Can you please try the latest code (from HEAD, copy and paste the new code - it's not published to NPM yet) and report if it works for you ?

glennschler commented 7 years ago

A possible suggestion for this https issue, is option to pass in an Agent object. Then no effort to decide inside cometd-nodes-client. Similar to the way Request module options work. The caller decides the many possible configurations of the http agent. Plus then will be able to instantiate many cometd clients per process with unique sandboxed agent objects. Also important, just like Request, allow optional Cookie store to be passed in.

I can separate my comments into Agent for issue #6, and another for Cookies.

Request modules optional options:

 

sbordet commented 7 years ago

@glennschler, the goal of this library is not to be a full rewrite of the browser CometD client, but just perform the necessary adaptations to make it work in a NodeJS environment. It basically has to just "fake" XMLHttpRequest.

From fixing #6, passing an Agent is not enough, since you have to switch on the http and https modules as well.

Cookies have been solved in #4.