PredictionIO-Node-SDK icon indicating copy to clipboard operation
PredictionIO-Node-SDK copied to clipboard

Library is fragile in baseUrl configuration

Open dmtrs opened this issue 11 years ago • 9 comments

var prediction = require('predictionio')({
    key: 'xxx',//add valid key here
    baseUrl: 'http://127.0.0.1:8000/'
});
prediction.users.get('nonexistinguserid', function (err, res) {
        console.log('err', err);
        console.log('res', res);
});

Results in

null
{}

This is because the request is structured as http://127.0.0.1:8000//users/nonexistinguserid.json with double // and the response of the server is

{ 
  //...
  links: {},
  text: 'Your request is not supported.',
  body: {},
  files: {},
  buffered: true,
  headers: 
   { 'content-type': 'text/plain; charset=utf-8',
     'access-control-allow-origin': '',
     connection: 'close',
     'content-length': '30' },
  header: 
   { 'content-type': 'text/plain; charset=utf-8',
     'access-control-allow-origin': '',
     connection: 'close',
     'content-length': '30' },
  statusCode: 404,
  status: 404,
  statusType: 4,
  info: false,
  ok: false,
  redirect: false,
  clientError: true,
  serverError: false,
  error: 
   { [Error: cannot GET //users/14.json?pio_appkey=xxx (404)]
     status: 404,
     method: 'GET',
     path: '//users/14.json?pio_appkey=xxx'
  }
  //...
}

dmtrs avatar Jul 15 '14 16:07 dmtrs

Hey

I will take a look and amend this. In the meantime you could remove the trailing / in the baseUrl to make it happy again.

Thanks

ShaunBaker avatar Jul 15 '14 16:07 ShaunBaker

I see in the code replication of this._config.baseUrl + '/users.json' there could be a function like this.url('users.json') or even this.url('users') to do the handling. what you think?

dmtrs avatar Jul 15 '14 16:07 dmtrs

Sounds good, would you like to fork and add?

ShaunBaker avatar Jul 15 '14 16:07 ShaunBaker

@ShaunBaker Currently having a look at the test suite

dmtrs avatar Jul 15 '14 16:07 dmtrs

From the test suite I can not see if the superagent is actually mocked. Are the test actually making requests to a localhost prediction.io instance?

dmtrs avatar Jul 15 '14 16:07 dmtrs

@dmtrs Yes the tests run against a local instance. When I originally worked on this I was pressed for time to mock etc!

ShaunBaker avatar Jul 15 '14 16:07 ShaunBaker

@ShaunBaker Do you have any preferences on a mock library in node? I have used http://sinonjs.org/ in some projects

dmtrs avatar Jul 15 '14 16:07 dmtrs

@dmtrs

Been super busy. Did you get anywhere with Sinon or something similar? Let me know and if not I will make some time to improve it.

Thanks

ShaunBaker avatar Aug 13 '14 22:08 ShaunBaker

Hey Shaun,

I am sorry but I did not have the time to do so. I would not be able to get on it till middle of September, unfortunately.

On Thu, Aug 14, 2014 at 1:27 AM, Shaun Baker [email protected] wrote:

@dmtrs https://github.com/dmtrs

Been super busy. Did you get anywhere with Sinon or something similar? Let me know and if not I will make some time to improve it.

Thanks

— Reply to this email directly or view it on GitHub https://github.com/ShaunBaker/PredictionIO-Node-SDK/issues/8#issuecomment-52120554 .

dmtrs avatar Aug 14 '14 10:08 dmtrs