jupyter / notebook

Jupyter Interactive Notebook
https://jupyter-notebook.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
11.79k stars 5.01k forks source link

Notebooks 5.x breaks widget calls #2977

Open rwmajor2 opened 7 years ago

rwmajor2 commented 7 years ago

We have developed our own custom widget that displays a map inside of a Jupyter Notebook. As part of that widget development, we use require.js for a few things. Prior to Notebooks 5.x, this has worked as expected. However, with Notebooks 5.1/5.2, we are now seeing poorly formed calls. For example, require.js used to make a call like the following:

http://js.arcgis.com/3.17amd/dijit/form/templates/TextBox.html?client=gsrs

And this works. However, with no changes in our Notebook widget call, the call now looks like:

http://js.arcgis.com/3.17amd/dijit/form/templates/TextBox.html.js?client=gsrs

It has appended a .js to the path. Has anyone seen this before? Can any suggest why this might be happening with the newer Notebook versions?

jcb91 commented 7 years ago

I would guess it's more likely to be down to the require version - requirejs is after all supposed to add .js extension to paths missing an extension altogether. How are you making the call in the first place?

rwmajor2 commented 7 years ago

Unfortunately, I am not an expert in this area @jcb91. Below is what it currently looks like:

require.config({
    // Define path mappings for modules
    paths: {
        // [1] Modules hosted on Esri CDN.
        "dojo": esriCDN + "dojo",
        "dojox": esriCDN + "dojox",
        "dijit": esriCDN + "dijit",
        "esri": esriCDN + "esri",
        "dgrid": esriCDN + "dgrid",
        "xstyle": esriCDN + "xstyle",
        "put-selector": esriCDN + "put-selector",
        "moment": esriCDN + "moment",
        "text": nbextensionPath + "/requirejs/text"
    },

    urlArgs: "client=gsrs",

    map: {
        "*": {
            "dojo/text": "text"
        }
    },
    config: {
        text: {

            useXhr: function (url) {
                return true;
            },
            openXhr: false,

            onXhr: function (xhr, url) {
                // Route cross domain XHR through a proxy if required
                var hasCors = (
                  typeof XMLHttpRequest !== "undefined"
                  && ("withCredentials" in (new XMLHttpRequest()))
                );

                xhr.open(
                  "GET",
                  hasCors ? url : (proxyUrl + "?" + url),
                  true
                );
            }
        }
    }
});
jcb91 commented 7 years ago

Ok, I can't say I'm an expert either really 😂 but the thing that stands out for me from the config you've posted is the proxying stuff - do you know whether you're actually going through a proxy? I suspect this SO question is relevant, it seems to be an issue regarding using the text plugin with cross-domain requests...

jcb91 commented 7 years ago

Ps, if it's the requirejs text plugin, newer versions of notebook already bundle it (and moment had been included for some time), so you could skip loading from the CDN/nbextension path altogether

rohitgeo commented 7 years ago

do you know whether you're actually going through a proxy?

We are not going through a proxy.

rohitgeo commented 7 years ago

Here's a screenshot showing what Chrome tries to load when bringing up the widget:

image

javascript files are getting loaded correctly, but some html files are being loaded with a ".js" extension, which fails. This is happenning from v5.1.0 onwards.

jcb91 commented 7 years ago

Well, without a minimal working example, my current best guess is:

  1. It seems that dijit is using the dojo/text require plugin to load html resources, as in the requirement at form/ValidationTextBox.js#L8.
  2. You've mapped dojo/text to text.
  3. In previous versions, the requirejs text plugin wasn't included in the notebook minified js, so as soon as something requested the text module, this would fetch your config's definition of nbextensionPath + "/requirejs/text". However, in newer versions (see this bit of 5.0.0 to 5.1.0 diff), the text module will already be defined as part of the main notebook js, so your config will (I think?!) get ignored. I suspect this may be relevant.

What version of requirejs/text are you providing? Have you set any config for it?

rohitgeo commented 7 years ago

We're providing RequireJS text 2.0.12. The config is what Bill had shared above.

rohitgeo commented 7 years ago

A minimal working example is attached. It used to work in notebook 5.0 but doesn't work in 5.1+ (tried with 5.2.1 as well).

HelloSceneWidget.zip

Your assessment above ("your config will get ignored") is spot on... from our debugging it seems that the require config for 'text' isn't being used - onXhr is never called. How can we work around this issue?

Thanks for your help with this.

jcb91 commented 7 years ago

Thanks @rohitgeo for that example. So, the simple way that I can get this to work is by just removing all of the requirejs text config along with the dojo/text mapping, so essentially just by using dojo/text as dojo & dijit expect.

The config call then looks like:

require.config({
    // Define path mappings for modules
    paths: {
        // [1] Modules hosted on Esri CDN.
        "dojo": esriCDN + "dojo",
        "dojox": esriCDN + "dojox",
        "dijit": esriCDN + "dijit",
        "esri": esriCDN + "esri",
        "dgrid": esriCDN + "dgrid",
        "xstyle": esriCDN + "xstyle",
        "put-selector": esriCDN + "put-selector",
        "moment": esriCDN + "moment",
    },
    urlArgs: "client=gsrs",
});

Was there a pressing reason that you wanted to use requirejs text instead of dojo's?

I tried removing your moment path-config, in favour of using notebook's builtin moment, but it appears some of your other cdn-hosted stuff is relying on finding things at moment/moment rather than the base moment defined in notebook. It also seemed that some cdn stuff may be doing some of the direct-requiring of moment locales I've tried to get rid of in #3048 (although I haven't had time to dig into your require calls enough to see where that was coming from).

jcb91 commented 7 years ago

Your assessment above ("your config will get ignored") is spot on... from our debugging it seems that the require config for 'text' isn't being used - onXhr is never called. How can we work around this issue?

As far as I can tell, the text plugin only reads its config once, at load time (it assumes all config-setting will be done before it's loaded - see text.js#L22). The only halfway-reasonable way I can think of to get around this is to undefine the module before loading it again to get the new config applied - see this SO answer for details on reloading modules.

This would then give something like

require.config({
  // my new config here
});
// unload the text plugin module
require.undef('text');
// now text should get loaded again by the next require call that asks for it:
require(['text!some/resource/wanted/here.html'], function (txt) {
    console.log('do something with', txt);
});
rohitgeo commented 7 years ago

Thank you @jcb91 for this fix of simplifying the require config. I believe we were using our version of requirejs/text as there's a small tweak we've done to it. However, it might have been for handling some niche case that I'm not aware of. I'd be more than happy to use this simpler require configuration. Thank you so much for your help!

Another thing I tried was to change "text" to "mytext", and it works!

jcb91 commented 7 years ago

No problem, glad there's a suitable solution for you :)

Another thing I tried was to change "text" to "mytext", and it works!

I'm not sure what you mean by this, which "text" did you change? And why did you use "mytext" for that matter, is that just arbitrary, to avoid colliding with the existing "text"?

rohitgeo commented 7 years ago

Hmmm... perhaps I spoke too soon. I just tried replacing the require config with the simple version you had but it's still not working for me. I'm wondering how it works for you. I used notebook v 5.2.1.

Please see the attachment below. HelloSceneWidgets_no_text_doesnt_work.zip

I replaced text to mytext (arbitrary name) to avoid colliding with the existing text. This is what I used as my require config:

require.config({
      // Define path mappings for modules
      paths: {
        // [1] Modules hosted on Esri CDN.
        "dojo":         esriCDN + "dojo",
        "dojox":        esriCDN + "dojox",
        "dijit":        esriCDN + "dijit",
        "esri":         esriCDN + "esri",
        "dgrid":        esriCDN + "dgrid",
        "xstyle":       esriCDN + "xstyle",
        "put-selector": esriCDN + "put-selector",
        "moment":       esriCDN + "moment",

        // [2] Modules hosted locally.
        // "location" is specified as path relative to web server root.
        "mytext":       "https://cdnjs.cloudflare.com/ajax/libs/require-text/2.0.12/text"
      },

      // Use RequireJS text plugin instead of dojo/text plugin.
      // Any module that requires dojo/text plugin will use RequireJS
      // text plugin instead.
      // http://requirejs.org/docs/api.html#config
      map: {
        "*": {
          "dojo/text": "mytext"
        }
      },

      config: {
        mytext: {

          useXhr: function(url) {
            // Allow cross domain XHR requests:
            // We will route them through a proxy in onXhr below.
            // https://github.com/requirejs/text/blob/master/text.js#L129

            return true;
          },

          // ESRI modification: let's take over xhr.open below
          openXhr: false,

          onXhr: function(xhr, url) {
              console.log("***onXhr");
            // Route cross domain XHR through a proxy if required
            var hasCors = (
              typeof XMLHttpRequest !== "undefined" 
              && ("withCredentials" in (new XMLHttpRequest()))
            );

            xhr.open("GET", hasCors ? url : (proxyUrl + "?" + url), true);
          }
        }
      } 
    });
jcb91 commented 7 years ago

Hmmm... perhaps I spoke too soon. I just tried replacing the require config with the simple version you had but it's still not working for me. I'm wondering how it works for you. I used notebook v 5.2.1.

Unfortunately, on that one, your guess is as good as mine for now! I always clear all outputs, save the notebook, reset the kernel & refresh the page after changing things, just to ensure nothing's stale. You can find the version that's worked for me in this gist...

I replaced text to mytext (arbitrary name) to avoid colliding with the existing text. This is what I used as my require config:

Ah, I see, to keep a custom mapping to another arbitrarily-named version of text, good idea!

edit:

I'm also on Jupyter 5.2.1, with ipywidgets 7.0.0