Use requirejs vs. require

[gnestor]

See jupyter/notebook#155

[jcb91]

Thanks @gnestor. Most of these I think actually don't need changing, since the require they use is actually a local variable provided by the define call's require dependency, so they should be unaffected by jupyter/notebook#155. The exceptions are toc2 & collapsible_headings, which use require.specified in the global context. Unfortunately, I think altering all of the instances would be a little more complicated than just using requirejs everywhere, since 'require' specified as a dependency for a define call gets handled as a 'special' module (see require.js#L573-L579 and require.js#L1157-L1162), and this wouldn't be the case if 'requirejs' is specified...

[jcb91]

not sure about this, it's a javascript kernel which is running this require call, so it likely doesn't want requirejs?

[gnestor]

Good catch! Correct.

[gnestor]

@jcb91 Thanks for the feedback. I haven't used requirejs or amd outside of the notebook project, so I don't understand the intricacies. I'm totally open to suggestions about how to best update this repo to reflect the changes in jupyter/notebook#155.

[jcb91]

@gnestor no problem.

I haven't used requirejs or amd outside of the notebook project, so I don't understand the intricacies

Nor have I really, but I've been bitten by a few of its intricacies in the commonjs wrapper style that got me to dig into how it behaves 😆

For almost all cases, I think we could get away with just doing nothing, given require is a local variable set as a function argument. But, I agree that switching to using requirejs as the variable name (as you've done) is probably a good idea, just to reduce confusion in comparison to notebook usage.

I've made a few fixes which I've pushed into this PR branch, hope that's ok. Essentially they are

1. to revert changes to third-party code items (codemirror & jqueryui modules, each of which have their own idioms for dealing with requirejs satisfactorily)

2. to revert a couple of inadvertent changes (requireModules got replaced with requirejsModules, which won't work).

3. Finally, there's one more 'magic' implementation detail which can bite here, which relates to the aforementioned commonjs wrapper define style. Explanation below...

The simplified commonjs wrapper is used when you have a call without explicit dependencies, like

define(function (require, exports, module) {
    var other_module = require('./my_custom_module');  // synchronous require call
}); 

in this format, requirejs actually searches the toString content of the definition function for calls like require('module_id_string') to infer dependencies that need to be available to the module. But, (because of the regex it uses), it won't find any using requirejs('module_id_string'), so in order to use the cjs style, we'd need to keep the locally-defined variable name as require. I think though, that the best option for nbextensions in such cases is simply to not use the commonjs style at all, since we don't actually need it, and it's more work & bug-prone for requirejs to do the string parsing anyway. I discovered this after trying to do something like

var my_dependency = './my_files/' + some_other_var;
require(my_dependency);

which also doesn't match the requirejs regex 🐛

[gnestor]

Thanks for making these changes, @jcb91!

in such cases is simply to not use the commonjs style at all

Does this require further refactoring then? Should it be done in this PR?

[jcb91]

@gnestor np! I think I've covered everything for this repo in this PR already in d668614 - there were only a few instances anyway.

There are a couple of dependencies which should also be dealt with, but they'll be handled in their respective repos anyway (e.g. minrk/nbextension-scratchpad#19, jfbercher/jupyter_latex_envs, jcb91/jupyter_highlight_selected_word#22).

In summary, I think this is ok to merge as-is, if anything breaks (hopefully not super likely!), we should hear about it soon enough before any pip/conda releases.