ShaunBaker / PredictionIO-Node-SDK

PredictionIO Node SDK
64 stars 9 forks source link

Library is fragile in baseUrl configuration #8

Open dmtrs opened 10 years ago

dmtrs commented 10 years ago
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'
  }
  //...
}
ShaunBaker commented 10 years ago

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

dmtrs commented 10 years ago

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?

ShaunBaker commented 10 years ago

Sounds good, would you like to fork and add?

dmtrs commented 10 years ago

@ShaunBaker Currently having a look at the test suite

dmtrs commented 10 years ago

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?

ShaunBaker commented 10 years ago

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

dmtrs commented 10 years ago

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

ShaunBaker commented 10 years ago

@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

dmtrs commented 10 years ago

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 notifications@github.com 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 .