Read remote file and CondRequire just in case of undefined

[vgsantoniazzi]

Add nb.readRemoteNotebookFile to read notebook from url.

Remove require call on broswer just in case of undefined.

[vgsantoniazzi]

@jsvine just resolved require for ansi_up and added support to load remote files.

What do you think about this PR?

[jsvine]

Hi @vgsantoniazzi, and thanks for suggesting this. I don't think this PR is a good fit for notebook.js — the readRemoteNotebookFile code is largely non-notebook specific and something better handled outside the library — but I'm glad to see you engaging with the library.

Some of your other suggestions in the PR are intriguing, but I don't quite understand the reasoning. I've left a couple of questions for you in the comments.

With appreciation, Jeremy

[vgsantoniazzi]

Hi @jsvine, thank you for review my PR.

I came from Rails world, where most of the libraries are fulfilled with a lot of features and are selected by the user when they want - while JS libraries are focused on solve just the core of the problem, and that is amazing.

About the PR, the main problem is getAnsi function, where the require is occurring on condRequire and browser tried to require a lib and this is available on root.ansi_up. require is not supported on browser. The piece of code where is on master, are not working on browser.

I'll open a new PR with just with this change, so will works fine on server and browser.

[jsvine]

Thanks @vgsantoniazzi. I still don't quite see how this:

var lib = root.ansi_up || condRequire("ansi_up");

... is functionally different than this:

var req = condRequire("ansi_up");
var lib = root.ansi_up || req;

Seems like they're equivalent, no?

Can you provide a test/script that demonstrates the difference/improvement?

[vgsantoniazzi]

Unfortunately no, because condRequire calls require https://github.com/jsvine/notebookjs/blob/master/notebook.js#L43 and this is not supported by the browser. And calls before check if root.ansi_up.

getMarkdown is a similar function (https://github.com/jsvine/notebookjs/blob/master/notebook.js#L46) that is written in the right form.

When executed in broswer, getMarkdown is loaded, but getAnsi is not, and break the library

[jsvine]

Hmmm, it seems to me that, if require is not present, then this line in condRequire ensures that require is not called:

return typeof require === "function" && require(module_name);

Again, a specific example/test would be helpful. But, in any case, I figure it couldn't hurt to make getMarkdown and getAnsi more similar to each other. I'll make the change, and add a thanks to you in the README.md. I really do appreciate you engaging with the codebase.

[jsvine]

Ah, after re-reading your comment, now I see what you're saying. My apologies for the confusion. That's a helpful fix, and has now been made in master. Thanks!

[vgsantoniazzi]

You're welcome!

To know the library importance, I've configured in our discourse forum and we're discussing a lot about DS / ML / AI tech using your code.

Thank you!