Skip to content

Loading…

start using require #5081

Closed
wants to merge 1 commit into from

5 participants

@Carreau

Just because at some point we will have to.

Starting with the easy stuff he last one with the less dependencies.

@minrk
IPython member

Hm, this is backwards from what I was expecting. I had thought we would include only <app>/main.js in a script tag, which would load everything else with require, rather than the other way around.

@Carreau

Hm, this is backwards from what I was expecting. I had thought we would include only /main.js in a script tag, which would load everything else with require, rather than the other way around.

Yes, almost, in the end. I tried to do it at once using shims, but it is not super straitforward.
Problem is that most of our javascript modules rely on IPython global to access other modules and mutate it.

require and define call works in a completly different manner wich is that the module have to define what it does require statically before the module definition being executed.

We could have in main

require('utils' , function(utils){
    require('cell', function(cell){
        require('codecell', function(codecell){
               .... IPython.utils = utils
               .... IPython...

But it is ugly.

I tried to put everything into a require.shim, but it does not prevent you from dooing the dependency graph.

And obviously converting scripts from the top down doesn't work because of the async nature of require.

So next steps would be to convert config.jsthen tooltip.jsthen ... one by one into dependence of either main.js or the other file they are needed for. Test, do the same for next file, until no more files are in templates.

@ellisonbg
IPython member

We are going to have to think very carefully about how to use require. We actually have two dependency graphs:

  • The actual source code dependency graphs. IOW, which .js files depend on which other .js files.
  • The instantiation of JS objects in the IPython namespace and references to those in various places.

It is not at all clear how to use require in this type of context.

@ellisonbg ellisonbg added this to the 3.0 milestone
@jasongrout
IPython member

I thought it was pretty clear. The js file dependency graph is immaterial since you really should be compressing all of the js files to one single js file (and require has optimizing tools to do this). So you only have to think about the object dependency graph, and that is done with define/require statements.

(and can I say that I can't wait for javascript ES6 modules?)

@ellisonbg
IPython member
@Carreau

Brian, I think you are missusing / missinterpreting what require does. Require is more like python import, and you are mixing what is Module an a Class defined in this module.
I know you know the difference, you are just probably mixing with the view you have a python import mechanism with the weird way javascript works.

When a file depends on a global instance of a class that sits somewhere
else (notebook.js depends on IPython.SaveWidget, etc.).

A file (I suppose you mean a module) should never rely on global. A file is just a file that define a module and exports and their dependencies.
If a module require globals you define a Module.init(...) or Module.configure() method that do not use prototype to set up anything.

IPython save_widget should never be used as a global, if a notebook js module instance need access to save_widget is should be passed as an argument to the constructor by whatever constructed it.

By the way, it does not really depend on save_widget, we shoudl just define a callback that is called on rename saved.

Tu reuse expressions you like, "As a theoretical physicist you need to abstract this" (this time it is not a sphere), and we have a "leaky abstraction" between what a Module is, the module once loaded/initiated and the instances.

Having this better decoupled will simplify dealing with file and the reuse of modules.

@ellisonbg
IPython member

Ok, let me try to summarize what you are saying...

  • We would get rid of the IPython JS namespace entirely. No globals.
  • When modules/files depend on each other, we use require.
  • When classes depend on instances of other classes, just pass the instance to the constructor.
  • main.js will still be the place where all of the instances are created and passed to each other.
  • Thus main.js will require the modules containing the class definitions it needs.

Is this an accurate summary?

@Carreau

We would get rid of the IPython JS namespace entirely. No globals.

Not using globals, we can still expose them fro convenience and backward compat :

require('foo', function(foo){
    IPython.foo = foo;
})

also requiring the same module should give you handle on the same module, so everythong shoudl still be accessible.

main.js would not require all, only what is needed to startup the notebook, the rest will be loaded via dependency. At the beginning I suppose we will have a lot in main.js but through isolation in the end main should be really shallow.

But you mostly describe well.

@ellisonbg
IPython member

@Carreau Can open an issue and summarize the main points of the design there. This PR itself to way too early to be meaningful. I would rather close the PR and track the discussion in an issue. This allows us to have a PR queue that is focused on stuff that is approaching merge readiness.

@jdfreder
IPython member

I had thought we would include only /main.js in a script tag, which would load everything else with require, rather than the other way around.

After playing with require.js I'm convinced that this is doable and I do think this is the right way.

We could have in main

require('utils' , function(utils){
require('cell', function(cell){
require('codecell', function(codecell){
.... IPython.utils = utils
.... IPython...
But it is ugly.

Require has support for multiple imports, I use it all the time in the widget stuff. I don't think it looks ugly at all when written like this:

require(['utils', 'cells', 'codecell'] , 
    function(utils, cell, codecell){

And obviously converting scripts from the top down doesn't work because of the async nature of require.

I'm not sure why you say this. Yes top-down wouldn't work for a

  • require
  • require
  • require

chain, but it does work just fine with a

  • require
  • define
  • define

chain (that's how I do it in the widget code, see /widgets/js/init.js).

@Carreau are you still planning on tackling this? I've been hacking away at the widget persistence stuff which would benefit greatly from require.js being implemented everywhere. (I use the r.js optimizer to "compile" and minify the dependency graph into one file.) If you don't have time to work on this or don't want to tackle it I can take over for you.

@Carreau
@Carreau

Closing as @jdfreder is taking care of it.

@Carreau Carreau closed this
@Carreau Carreau deleted the Carreau:start-req branch
@minrk minrk modified the milestone: 3.0, no action
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 9, 2014
  1. @Carreau

    start using require

    Carreau committed
Showing with 13 additions and 7 deletions.
  1. +7 −6 IPython/html/static/notebook/js/main.js
  2. +6 −1 IPython/html/templates/notebook.html
View
13 IPython/html/static/notebook/js/main.js
@@ -13,12 +13,12 @@
// as injecting require.js make marked not to put itself in the globals,
// which make both this file fail at setting marked configuration, and textcell.js
// which search marked into global.
-require(['components/marked/lib/marked',
+define(['components/marked/lib/marked',
'notebook/js/widgets/init'],
-
-function (marked) {
- "use strict";
-
+function (marked, widgetinit) {
+"use strict";
+return {
+ init: function(val){
window.marked = marked;
// monkey patch CM to be able to syntax highlight cell magics
@@ -119,4 +119,5 @@ function (marked) {
}
});
}
-});
+}
+}});
View
7 IPython/html/templates/notebook.html
@@ -341,7 +341,6 @@
<script src="{{ static_url("notebook/js/notificationarea.js") }}" type="text/javascript" charset="utf-8"></script>
<script src="{{ static_url("notebook/js/tooltip.js") }}" type="text/javascript" charset="utf-8"></script>
<script src="{{ static_url("notebook/js/config.js") }}" type="text/javascript" charset="utf-8"></script>
-<script src="{{ static_url("notebook/js/main.js") }}" type="text/javascript" charset="utf-8"></script>
<script src="{{ static_url("notebook/js/contexthint.js") }}" charset="utf-8"></script>
@@ -349,4 +348,10 @@
<script src="{{ static_url("notebook/js/celltoolbarpresets/rawcell.js") }}" type="text/javascript" charset="utf-8"></script>
<script src="{{ static_url("notebook/js/celltoolbarpresets/slideshow.js") }}" type="text/javascript" charset="utf-8"></script>
+<script type="text/javascript" charset="utf-8">
+ require(["notebook/js/main"], function(app){
+ app.init()
+ });
+</script>
+
{% endblock %}
Something went wrong with that request. Please try again.