Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding support for AMD with RequireJS #56

Merged
merged 3 commits into from May 29, 2014

Conversation

stephen-james
Copy link
Contributor

Using RequireJS to define modules that require dimple is complicated by the fact that d3 doesn't register itself in the global namespace if it detects AMD/RequireJS.

// sample from d3's end.js....

if (typeof define === "function" && define.amd) {
    define(d3);
  } else if (typeof module === "object" && module.exports) {
    module.exports = d3;
  } else {
    this.d3 = d3;
  }

So while it is possible to "shim" dimple using RequireJS, the d3 global reference that dimple relies on is missing (even though the d3 module is loaded).

One approach to solving this would be to wrap the entire dimple creation code in a function that serves d3 to dimple via closure. (below example code untested!)

// Wrap all application code in a self-executing function
(function () {
    "use strict";

    var factory = function(d3) {
        // Create the stub object
        var dimple = {
            version: "1.1.5",
            plot: {},
            aggregateMethod: {}
        };

    // IMPLEMENTATION HERE

        return dimple;
    }; // end of factory

    if (typeof define === "function" && define.amd) {
        define(["d3"], factory);
    } else if (typeof module === "object" && module.exports) {
        var d3 = require("d3");
        module.exports = factory(d3);
    } else {
        this.dimple = factory(this.d3);
    }
}());
// End dimple

... but due to wrapping everything in a second level function (factory) all script files would have to be indented further, or we'd have to modify the grunt task to do that indentation on build. I'm also not convinced that this is most efficient.

The option presented in this pull request is a load simpler, it retrofits d3 in the global namespace if RequireJS is detected (perhaps bad for namespace pollution, but negligably?) and registers dimple both in the global namespace and as a module.

the answer to the following stack overflow question might help weigh this up....

http://stackoverflow.com/questions/17936854/when-designing-a-js-library-should-i-make-it-requirejs-amd-compatible-or-not

@johnkiernander
Copy link
Member

Among all the changes in the refactoring for testability branch (https://github.com/PMSI-AlignAlytics/dimple/tree/refactoring-for-testability) I have moved each method into its own closure and built the objects using a prototype model. It's a much nicer approach and means that each js file is well formed. Would that make it cleaner to implement this? If so we should make that change to the master branch, the refactoring branch is massive and won't be getting merged for a while.

@stephen-james
Copy link
Contributor Author

Sure, I'll take a look and see how it would fit in with the new structure - also just trying to get some tests going and have it play nicely with karma... Will shout when done!

@davclark
Copy link

davclark commented Mar 4, 2014

For those of us who are struggling using shims... how do you get shims to work for dimple.js exactly?

You can now answer this on stackoverflow and get a few points!

@stephen-james
Copy link
Contributor Author

Hi @davclark,

A bit of background from the RequireJS site

"Ideally the scripts you load will be modules that are defined by calling define(). However, you may need to use some traditional/legacy "browser globals" scripts that do not express their dependencies via define(). For those, you can use the shim config. To properly express their dependencies."

Ok, now to jump in to the problem... To create a RequireJS shim for dimple has become a bit more complicated recently because recent versions of d3 (as you know, a dependency of dimple) hides itself from the global namespace (which is a good practice) if it detects that RequireJS/AMD is being used.

Now currently dimple relies on browser globals (for example it expects d3 to be registered in the global namespace) and it also registers itself in the global namespace too. So the first part of using dimple with RequireJS is to make sure you reference a version which supports AMD. At the time that I'm writing this, no version on the original repo does, but you can modify your dimple.js and add the following to it :

// AMD support for RequireJs
(function (globalContext) {
    "use strict";

    if (typeof define === "function" && define.amd) {
        // d3 does not register itself within the global namespace if it detects AMD
        // yet dimple relies on d3's availability globally.
        define(["d3"], function (d3) {
            // register d3 in the global namespace so that dimple can use it
            globalContext.d3 = d3;
            // return the dimple object as the module
            return globalContext.dimple;
        });
    }
 }(this));

As per the pull request you're commenting on.

Save the modified dimple.js file with the suffix ".amd" Great now thats out of the way, d3 will be available in the global namespace and dimple will be defined so we can go ahead and make our shim.

In your requirejs config, add paths to your AMD modified version of dimple and the normal d3 script. And add a shim to specify that dimple depends on d3 :

requirejs.config({
    paths : {
        "dimple" : "lib/dimple.v1.1.5.amd",
        "d3" : "lib/d3",
    },

    shim : {
        "dimple" : {
            deps : [ "d3" ],
            exports : "dimple"
        }
    }
}); 

Thats it, should be all there is to it.

You can now write modules that require dimple and everything should load up nicely.

Yes the global namespace has dimple and d3 in it, but thats not the end of the world. (imho) although we are thinking of a way to structure dimple to be fully AMD compatible, it's going to have quite an impact on the grunt build of the project and how things are structured so it's unlikely it will be available soon. In the meantime, this work-around will let you use dimple with RequireJS.

You can find out more about RequireJS shims from the RequireJS site, they'll probably do a whole lot better at explaining this than I did!

@davclark
Copy link

davclark commented Mar 6, 2014

That was very helpful - I wasn't thinking of a shim as being a library, I was just thinking of it as a config call. I did read the docs on the RequireJS site, and your explanation added a lot to my understanding. Thank you!

@stephen-james
Copy link
Contributor Author

Glad it helped!  How did you get on in the end?  Did you get to a solution?  I saw from your stackoverflow question you're wanting to use it with IPython notebook - I'm really interested in that so if you get a chance reply on that stack thread and we can take a look!

Sent from Mailbox for iPhone

On Thu, Mar 6, 2014 at 11:44 PM, Dav Clark notifications@github.com
wrote:

That was very helpful - I wasn't thinking of a shim as being a library, I was just thinking of it as a config call. I did read the docs on the RequireJS site, and your explanation added a lot to my understanding. Thank you!

Reply to this email directly or view it on GitHub:
#56 (comment)

@rdhyee
Copy link

rdhyee commented Mar 12, 2014

I followed the instructions for creating a dimple.amd specified by @stephen-james and embedded dimplejs (based on @davclark's example) into an IPython notebook: http://nbviewer.ipython.org/gist/anonymous/9501035.

The dimple graphs show up, but the hover boxes are empty. I don't know whether it's some interaction with ipynb css with the dimple css that's the problem.

@stephen-james
Copy link
Contributor Author

I think the empty hovers are related to an issue with dimple's tooltip class clashing with bootstrap, but John will be able to advise better for this one!  Great to hear you got it working in IPython notebook, I'll take a look - am sure some of the guys here will find that really useful

Sent from Mailbox for iPhone

On Wed, Mar 12, 2014 at 10:55 PM, Raymond Yee notifications@github.com
wrote:

I followed the instructions for creating a dimple.amd specified by @stephen-james and embedded dimplejs (based on @davclark's example into an IPython notebook: http://nbviewer.ipython.org/gist/anonymous/9501035.

The dimple graphs show up, but the hover boxes are empty. I don't know whether it's some interaction with ipynb css with the dimple css that's the problem.

Reply to this email directly or view it on GitHub:
#56 (comment)

@johnkiernander
Copy link
Member

Yes, just add the following to your CSS:

.tooltip {
    opacity: 1 !Important
}

It's a clash with bootstrap as Stephen says.

More detail here: #34

@rdhyee
Copy link

rdhyee commented Mar 13, 2014

@rdhyee
Copy link

rdhyee commented Mar 14, 2014

I learned when showing the notebook to my students that the demonstration works with the current IPython 2.0 beta but not IPython 1.2.1. So back to the drawing board to figure out the reasons why.

@davclark
Copy link

While it may not be worth it now that 2.0 is released, @minrk can likely provide the explanation.

@johnkiernander
Copy link
Member

Stephen, I decided against the code refactor in v2.0 in the end due to the problems it would cause with adding requirejs support. Can you try this out with dimple v2.0, if it still works which I'm sure it will, let me know and I'll merge it for the next release.

@stephen-james
Copy link
Contributor Author

Sure I'll give it a go this evening

@johnkiernander johnkiernander merged commit 7c5f3f0 into PMSI-AlignAlytics:master May 29, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants