n-riesco / ijavascript

IJavascript is a javascript kernel for the Jupyter notebook
Other
2.18k stars 187 forks source link

feat(bin): provide ijskernel as bin #84

Closed rgbkrk closed 7 years ago

rgbkrk commented 8 years ago

This makes it so that ijskernel can be used directly as a bin script, which makes my local kernel.json much easier to work with when I'm changing node versions:

{
  "argv": [
    "ijskernel",
    "{connection_file}",
    "--protocol=5.0"
  ],
  "display_name": "Javascript (Node.js)",
  "language": "javascript"
}

For me, this makes #67 much easier to deal with. Now, every time I upgrade node with nvm, I globally install ijavascript again and it just works because it's not reliant on a hardcoded path to my node version + the library location.

On windows, the kernelspec is:

{
  "argv": [
    "ijskernel.cmd",
    "{connection_file}",
    "--protocol=5.0"
  ],
  "display_name": "Javascript (Node.js)",
  "language": "javascript"
}

/cc @thisgeek @c22

rgbkrk commented 8 years ago

If you're happy with this, then I'd love to adapt install to set this as the kernelspec as well.

c22 commented 8 years ago

Neat solution!

n-riesco commented 7 years ago

@rgbkrk Sorry for the late response. This approach is a very nice solution.

As you mentioned above, the spec needs updating. To do so, change these lines to:

    context.args.kernel = [ 
        (process.platform === 'win32) ? 'ijskernel.cmd' : 'ijskernel',
    ];
rgbkrk commented 7 years ago

Made the change you requested @n-riesco

n-riesco commented 7 years ago

I've tested this PR and I've run into problems when ijskernel isn't installed in one of the folders listed in PATH.

I've tried to use __dirname, but it points to the IJavascript folder (it doesn't point to the .bin folder where ijskernel is installed).

This is how I suggest that we proceed:

If you don't have the time, I can do this second PR.

rgbkrk commented 7 years ago

If you want to add commits on to this one, that's a-ok with me (allow edits from maintainers is checked). I can also tackle this - commit c44c074 was introduced based on your feedback. I'd love to see the ijskernel in the kernelspec when kernel installed globally part, haven't looked into how that would work here.

n-riesco commented 7 years ago

@rgbkrk To be clear, I'm not opposed to merging this PR. I'm only trying to avoid regressions.

After some more thought, I don't think overloading the meaning of --ijs-install=global is a good a idea, because installing a kernel spec globally doesn't ensure the kernel binary has also been installed globally.

This is what I'm planning to do:

  1. Merge this PR.
  2. Revert 44c074
  3. Introduce the flag --ijs-spec-path=[none|full]. Since in README.md I'm recommending a global installation (sudo npm install -g ijavascript) and thus --ijs-spec-path=none should work, I'm planning to use none as a default.

Before I publish a new release, I'd like to hear any arguments against using --ijs-spec-path=none as a default.

At the moment, these are the only arguments I've thought of:

rgbkrk commented 7 years ago

I haven't been using any of these options, though it seems I've needed them -- I've been running ijs then closing the notebook server that comes up to install the kernel.

rgbkrk commented 7 years ago

Thanks for the discussion on this and paving a path forward.

n-riesco commented 7 years ago

@rgbkrk Another flag that it's very convenient to preload libraries in Node session is --ijs-startup-script=[file.js|folder]