vex.close() undefined under CommonJS/Browserify

[ngault]

I’m creating a new issue about this because I notice that adamschwartz just merged hugomrdias’ PR 71, making NPM distribution a possibility…which is great. Thank you!

But… it doesn’t appear that folks realize that using vex.dialog.js with CommonJS/Browserify won’t work in the case of (by my count) about 60% of all Vex examples posted by Adam over date because of CommonJS namespacing.

In particular, if you require under CommonJS vex.dialog.js,the key methodvex.close() is undefined. (There is no such thing as vex.dialog.close(),of course.) To reproduce the problem, try Browserify'ing some of Adam's JSFiddle examples, such as:

http://jsfiddle.net/adamschwartz/Lrq84/

or

http://jsfiddle.net/adamschwartz/r27K5/show/light/
which is described in depth here
#59

Vex currently offers:

   vex.dialog.alert()
   vex.dialog.confirm()
   vex.dialog.prompt()
   vex.dialog.open()

As a solution to this namespacing problem, my personal taste would NOT be to make vex.close() available to the vex.dialog object, but rather to create a new method named vex.dialog.close(). But I'll be happy for any fix that will allow me to Browserify my existing Vex dialogs, which are dependent on vex.close()

[hugomrdias]

ill check this out

[adamschwartz]

Thanks @ngault and @hugomrdias

[hugomrdias]

can you post a snippet of your code here pls ?
cant replicate the problem

[ngault]

hugomrdias, here's a gist with the CommonJS wrapped version of Adam's login example, packaged, as it must be, with its dependencies.

At the bottom of this (sorry) long post, I provide a version of stripped of wrapper and dependencies.

DevTools showing the example code choking on vex.close():

A quick recap of my take on the problem because I may have been unclear in my original description:

Consistent with the notes to your PR, to require vex.dialog.js, CommonJS won't permit module assignment to "dot" denoted objects. In other words, as much as we'd like to re-use Aaron's pattern from his non-CommonJS examples, we cannot do this:
var vex.dialog = require('./vex.dialog.js');

Instead, as you do in your PR, we must assign the module like this:
var dialog = require('./vex.dialog.js');
... making the following statement (which works in all Aaron's examples not using a CommonJS wrapper) undefined:
vex.close($vexContent.data().vex.id);

...and, as we know, there is no such function in vex as:
vex.dialog.close($vexContent.data().vex.id);

End of recap. Here's the code (exactly the same as Adam's, except for the require assignment, for which we are restricted) that will fail under CommonJS.

var VexDialog = require('./vex.dialog.js');

(function() {
    var login;

    login = function(email, password, callback) {
        return setTimeout(function() {
            if (email === 'test@test.com' && password === 'test') {
                return callback('success');
            } else {
                return callback('An error occurred logging in. Try email `test@test.com` and password `test`.');
            }
        });
    };

    VexDialog.prompt({
        appendLocation: '.header-interior-leftmost-pad',
        className: 'vex-theme-wireframe',
        message: 'Use "test@test.com" with password "test" ',
        input: '<input name="email" type="email" class="vex-dialog-prompt-input" placeholder="email@example.com" value="" required>\n<input name="password" type="password" class="vex-dialog-prompt-input" placeholder="password" value="" required>',

        onSubmit: function(event) {
            var $vexContent;
            event.preventDefault();
            event.stopPropagation();
            $vexContent = $(this).parent();
            return login(this.email.value, this.password.value, function(message) {
                if (message === 'success') {
                // ***** PROBLEM HERE:******** vex is undefined
                    return vex.close($vexContent.data().vex.id);
                } else {
                    return console.error(message);
                }
            });
        }
    });

[hugomrdias]

i didn't setup a local demo with your gist but i think i got the problem.

1. why are you require'ing like this var VexDialog = require('./vex.dialog.js'); ?
   you should install vex with npm and require with a package relative path like i said in the PR.

var dialog     = require('vex/js/vex.dialog.js');

1. now to solve your problem check this gist

'use strict';

var  vex        = require('vex'),
       VexDialog     = require('vex/js/vex.dialog.js');

VexDialog.prompt({
        appendLocation: '.header-interior-leftmost-pad',
        className: 'vex-theme-wireframe',
        message: 'Use "test@test.com" with password "test" ',
        input: '<input name="email" type="email" class="vex-dialog-prompt-input" placeholder="email@example.com" value="" required>\n<input name="password" type="password" class="vex-dialog-prompt-input" placeholder="password" value="" required>',

        onSubmit: function(event) {
            var $vexContent;
            event.preventDefault();
            event.stopPropagation();
            $vexContent = $(this).parent();
            return login(this.email.value, this.password.value, function(message) {
                if (message === 'success') {
                // now vex should be available 
                    return vex.close($vexContent.data().vex.id);
                } else {
                    return console.error(message);
                }
            });
        }
    });

vex.dialog doesn't work like a jquery plugin it doesn't attach it self to the main vex object instead it requires vex instance and calls it with specific options for .alert, .confirm etc etc.
so if you need to call vex.close() you need the vex instance and not the vex.dialog to do so you need to require both as you can see in the above gist.

vex is a singleton holding all the active dialogs, vex.dialog is just static methods calling vex with different options.

Please follow these instructions and report back, hopefully im right if not ill have a second look a it.

@adamschwartz what you think about registering with npm and maybe adding some docs for commonjs enviroments

[adamschwartz]

Thanks for pushing on this.

@hugomrdias As I don’t use CommonJS this stuff is a little unfamiliar to me. That being said I’m happy to merge any PR’s to the docs which help people understand what’s going on.

Currently the docs contain this message:

Does that not fully explain the situation? Again, sorry if I’m missing something here.

@timmfin also wondering if you have any thoughts here.

[ngault]

@adamschwartz , thanks for pointing out that the docs instruct CommonJS users of vex.dialog.js to NOT also require vex.js. You are not missing anything, this is precisely the issue. PR 71 and the docs are not compatible.

I believe that the current documentation describes the way that a CommonJS/npm of vex.dialog.js SHOULD work: the use of a single module should not demand explicit statement of its dependencies. (In that sense, CommonJS is unlike AMD.) That's what npm users expect.

So, if you want a fix that follows CommonJS/npm convention, I'd leave the documentation as is and decide upon the best way to make available vex.close().

@hugomrdias, granting the accuracy of your explanation about how multiple vex.dialog objects attach themselves to the single vex object, I'd argue that this is an implementation detail that a user of the library shouldn't be expected to know. I'd argue that a more important consideration to the design of your solution is the comprehensibility of the API.

[hugomrdias]

@ngault i reviewed the code again and what i said earlier isnt 100% accurate, vex and vex.dialog are just objects with static methods they always check the DOM for the dialogs, etc..
To make vex.dialog self contained the code needs some refactoring to work across the 3 environments, because we cant easily change the return values or just extend vex with vex.dialog method names would clash and stuff like that.

its up to @adamschwartz because i dont do coffee script :) sorry

personally i'm happy for now it works with cjs/browserify even if we need to require both files.

[ngault]

Hugo, thanks for looking into this and getting back to me so fast. I agree, the most important thing is that with your PR we can now use vex.dialog.js under CommonJS/npm/browserify. Adam will just need to change the docs, otherwise anybody using the new vex.dialog.js will get stuck.

[bbatliner]

Hey all! See my comment on #60 and let me know if your issue is resolved! Thanks!

[bbatliner]

Assuming this is ok now, we've had standalone module support for a while.