Api changeup

[rgbkrk]

Primary tweaks I made here were:

• Call out directly to the underlying kernelspecs and kernel functions (it was returning a bound function instead)
• The ajax calls should not map to the response, in case the user needs the status code, the headers, etc. - if we figure out that there's a common set of transformations to apply, we can provide those once we know about them

[codecov-io]

Current coverage is 76.59% (diff: 79.41%)

Merging #5 into master will increase coverage by 3.51%

@@             master         #5   diff @@
==========================================
  Files             1          3     +2   
  Lines            26         47    +21   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits             19         36    +17   
- Misses            7         11     +4   
  Partials          0          0          

Powered by Codecov. Last update a9e337c...3248745

[rgbkrk]

More editorial from me: I've made the endpoint be the base path from Jupyter (this is a known value by tmpnb, JupyterHub, etc.)

[rgbkrk]

This changes the naming structure to be a bit simpler (and verbose where necessary). I went through quite a few revisions before rebasing.

Usage:

const jupyter = new rxJupyter.JupyterAPI("http://localhost:8888", true)

jupyter.listKernelspecs()
  .pluck('response')
  .subscribe(kernelspecs => { console.log(kernelspecs) })

It is a bit annoying to return the ajax response, but then it's the only way to work with errors and status codes effectively (which will happen). 🤔

[rgbkrk]

@willingc Thanks for diving in! One strangeness we have is that some of these functions are effectively returning functions (the observables), which means the resulting verbs haven't even occurred yet. I'd love to figure out good naming with you for our Rx stuff, I keep iterating on this stuff with a bit of uncertainty.

[rgbkrk]

I'm really tempted to be passing around a serverConfig object (plain object), like this:

const serverConfig = {
  endpoint: 'http://127.0.0.1:8888',
  crossDomain: true,
};

Then passing that to e.g. kernels.list:

kernels.list(serverConfig);

This would be in lieu of (or in addition to) the JupyterAPI Object. Especially as we end up having to handle auth or other server options, we don't want to expand the amount of arguments to each function for each resource.

[rgbkrk]

Awww yeah, 100% coverage.

[rgbkrk]

Here's how it works now:

const jupyter = require('rx-jupyter');

const serverConfig = {
  endpoint: "http://localhost:8888",
  crossDomain: true,
};

jupyter.kernelspecs.list(serverConfig)
  .subscribe(ajaxReply => {
    console.log(ajaxReply.response);
  });

[rgbkrk]

Ok, I'm in love with this API as it stands for calling - works well in a functional programming environment and reads idiomatically.

The responses however... I'm not thrilled about. To make it ergonomic, I'd want to return the response directly but then we lose the status code.

/cc @jayphelps any ideas?

[rgbkrk]

Now with full eslinting, including on the tests.

[jayphelps]

@rgbkrk it's not clear why you need to pass a serverconfig object instead of just the endpoint? The crossDomain property is used for two possible reasons, the first being whether to set the withCredentials property and the most important reason is whether or not you're targeting IE 8 or 9 which had to use XDomainRequest for CORS. If this isn't applicable, it sounds like you could just pass the endpoint itself

[jayphelps]

@rgbkrk ah I missed this:

Especially as we end up having to handle auth or other server options, we don't want to expand the amount of arguments to each function for each resource.

that makes sense. If you need to pass auth too. If the auth doesn't change commonly within a session, you might have a factory/class that you provide the config to and then it gives you API methods you can call without needing to repeatedly pass around the same things.

[rgbkrk]

We'll likely be using this from a separate domain and if crossDomain isn't set, we get an error:

[rgbkrk]

If the auth doesn't change commonly within a session, you might have a factory/class that you provide the config to and then it gives you API methods you can call without needing to repeatedly pass around the same things.

Amusingly, that's what was here originally (a top-level JupyterAPI). Since it was simply calling on to these functions, testing it felt a bit silly. Then again, I see how this could be done in one factory function, binding every function within each resource.

[jayphelps]

We'll likely be using this from a separate domain and if crossDomain isn't set, we get an error:

Shit yeah I didn't realize RxJS is injecting that for requests not marked as cross-domain. While I haven't tested, looking through the code it appears it wouldn't cause any harm to set crossDomain: true always, even if not really cross domain. In modern browsers the only difference I see off hand is that it wouldn't include that X-Requested-With header, which I've never seen provide any usefulness.

The responses however... I'm not thrilled about. To make it ergonomic, I'd want to return the response directly but then we lose the status code.

I struggle with too all the freaking time. 99% of the time I don't need the status code or other meta but then when I do need it I have to break convention. Lately I've been doing effectively the same as you, just letting the full object pass through and they can pluck response from it consistently. With param destructuring, it hasn't been a big deal.

[rgbkrk]

Shit yeah I didn't realize RxJS is injecting that for requests not marked as cross-domain.

Oh yeah, I remember trying to change the headers directly when @captainsafia were playing with this together at the NumFocus Summit. Ended up resorting to crossDomain: true. Totally forgot why.

99% of the time I don't need the status code or other meta but then when I do need it I have to break convention. Lately I've been doing effectively the same as you, just letting the full object pass through and they can pluck response from it consistently.

Welp, that's what I expected. Thanks for chiming in.

Super extra special thanks for commenting on this and helping out on a Sunday!

[jayphelps]

Amusingly, that's what was here originally (a top-level JupyterAPI). Since it was simply calling on to these functions, testing it felt a bit silly. Then again, I see how this could be done in one factory function, binding every function within each resource.

to clarify--I was meaning something like this:

export class API {
  constructor(serverConfig) {
    this.serverConfig = serverConfig;
  }

  listKernals() {
    return ajax(createSettingsForList(this.serverConfig, 'kernels'));
  }

  listKernalSpecs() {
    return ajax(createSettingsForList(serverConfig, 'kernelspecs'));
  }

  getKernal(id) {
    return ajax(createSettingsForGet(this.serverConfig, id));
  }

  startKernal(name, path) {
    return ajax(createSettingsForStart(this.serverConfig, name, path));
  }
}

// later

const jupyter = require('rx-jupyter');
const api = new jupyter.API({ endpoint: 'http://suchwow', crossDomain: true });

api.getKernal('123')
  .subscribe(({ response }) => console.log(response));

previously the class just deferred each method to functions via bind, but it's not clear why.

[jayphelps]

which again hilariously appears nearly identical to what you have in your original comment on this PR

const jupyter = new rxJupyter.JupyterAPI("http://localhost:8888", true)

jupyter.listKernelspecs()
  .pluck('response')
  .subscribe(kernelspecs => { console.log(kernelspecs) })

I'm too removed from this domain to know what's the most ergonomic, so definitely go with your gut.