Loading spectrum via a script loader (e.g. yepnope)

[conzett]

I am trying to load spectrum via modernizr and yepnope. It loads spectrum.js correctly, but throws the error "tinycolor is not defined" on line 433. Is there a preferred way to load spectrum as a conditional polyfill. Testing in FireFox 16 Chromium Debian.

[bgrins]

That is surprising. Can you send a jsbin or jsfiddle example? I don't have any experience with yepnope, so I'll be interested to see how that loading happens.

[bgrins]

tinycolor is exposed before any spectrum instances are created as far as I know: https://github.com/bgrins/spectrum/blob/master/spectrum.js#L1576. Does the problem go away if you move the TinyColor declaration up in the file?

[conzett]

Thanks for the fast reply, here is a jsfiddle that demonstrates the error: http://jsfiddle.net/jZxNR/5/ I wonder if it has anything to do with the asynchronous nature of yepnope's loading?

[bgrins]

This is really weird, but I got it working by wrapping everything in (function() {})(); http://jsfiddle.net/jZxNR/6/. I'm guessing that yepnope is pulling down the script as text and evalling it or something? That is weird. Either way, I can just move the tinycolor declaration inside of the first wrapped function.

[bgrins]

But can you confirm that the new fiddle is working?

[conzett]

Strange, I'm still getting the error in that fiddle (revision 6). Testing now on OS X, still in FireFox 16.0

[bgrins]

Oh that is weird - for some reason the document ready call here https://github.com/bgrins/spectrum/blob/master/spectrum.js#L786 is firing before the rest of the script evaluates (when I move that to the bottom of the file the issue goes away). I guess because the document is ready when yepnope loads. I can move that call to the bottom of the file.

Try this one: http://jsfiddle.net/jZxNR/7/

[conzett]

That seemed to do the trick! Will that change be making its way into master or should I use the yepnope branch?

[bgrins]

I'll merge it into master so it will work for everyone

[bgrins]

Can you confirm that it is working now on master on your original link: http://jsfiddle.net/jZxNR/5/?

[conzett]

Yes, it is working on the original fiddle. Thanks so much for your work on this, best colorpicker component I've found and a huge plus that it doesn't use images and works easily as a shim/pollyfill.

[bgrins]

Thanks!

[fadomire]

Is it possible to merge this into master ? i got the same problem with requireJS. Thanks

[bgrins]

I thought this was working. The original fiddle has some broken CSS/JS links, but when I updated those it seemed to work: http://jsfiddle.net/bgrins/jZxNR/12/. Can you send a link with it failing on requireJS?

[fadomire]

maybe there is something i'm missing but trying to use spectrum with require js give 'tinycolor is not defined'

see http://jsfiddle.net/efMmB/9/

[bgrins]

Github changed their pages to not serve scripts, so I had to change the URL to http://bgrins.github.com/spectrum/spectrum.js and it seems to work. Are you seeing this too?

http://jsfiddle.net/bgrins/efMmB/10/

[fadomire]

it works with your link but the version of the libs are not the same. So i guess it is failing with the last one only.

Not working version : v0.9.16
Working version : v0.9.14

[fadomire]

Most notable change is tinycolor that dont export his object globally :
0.9.14 : root.tinycolor = tinycolor;
0.9.16 :
// Node: Export function
if (typeof module !== "undefined" && module.exports) {
module.exports = tinycolor;
}
// AMD/requirejs: Define the module
else if (typeof define !== "undefined") {
define(function () {return tinycolor;});
}
// Browser: Expose to window
else {
window.tinycolor = tinycolor;
}

[fadomire]

related to #113 and your last commit i guess

[bgrins]

wait, so window.tinycolor = tinycolor; doesn't work with requirejs? root was passed in as this, which typically would have been window anyway. So if you change the code back to how it was before the last commit, everything works as expected with requirejs?

[fadomire]

it is not that window.tinycolor = tinycolor is not working with requirejs.
In fact when requirejs is detected by tinycolor this line is not executed and instead this one is :
define(function () {return tinycolor;});

In previous versions of tinycolor it was always exporting window.tinycolor making spectrum able to use it.

I dont know if you are up to or if there is a specific reason you did not do it but i guess i would suggest to treat tinycolor as a dependency and put it in a libs/ folder removing it from core source. Same as jquery.

So non requirejs user would import jquery, tinycolor, spectrum in their html.

For requirejs (and other AMD loader) to be able to use spectrum correctly you would also need to replace current anonymous function that wrap your plugin to something like that :

(function (factory) {
    if (typeof define === 'function' && define.amd) {
        // AMD. Register as an anonymous module.
        define(['jquery', 'tinycolor'], factory);
    } else {
        // Browser globals
        factory(jQuery, tinycolor);
    }
}(function ($, tinycolor) {
    // your core code goes here
}));

it is just a suggestion, and i understand it can be some work.

I could make a fork to demonstrate how it could work if you want. Let me know.

[bgrins]

OK, I think I see. So for the spectrum version of tinycolor, we really need to just remove that whole if/else block and just set window.tinycolor. This is not going to be used in any non-browser context anyway, and it will not be loaded separately from spectrum. I'd rather do that than separate files.

Would the problem be fixed by replacing this:

// Node: Export function
if (typeof module !== "undefined" && module.exports) {
    module.exports = tinycolor;
}
// AMD/requirejs: Define the module
else if (typeof define !== "undefined") {
    define(function () {return tinycolor;});
}
// Browser: Expose to window
else {
    window.tinycolor = tinycolor;
}

With this?

window.tinycolor = tinycolor;

[fadomire]

Yes replacing whole if/else block with only window.tinycolor = tinycolor; will solve the problem.

But each time you update tinycolor you will need to remember to make that change again. Again managing tinycolor as a dependency would be more bulletproof imo, but you are the maintainer :)

Thanks for your plugin and implication.

[bgrins]

But each time you update tinycolor you will need to remember to make that change again. Again managing tinycolor as a dependency would be more bulletproof imo

Yes, and that is a concern with implementing it this way. However, I don't want to split the development into multiple files because I think having it in a single file makes the usage instructions more simple. Can you think of any way to make the define call work without splitting into multiple files but still exposing the tinycolor object to the window?

[fadomire]

i guess you could keep the code as it is. I just found that it is sufficient to make tinycolor a global object in the requirejs callback.

See http://jsfiddle.net/efMmB/17/

current code is working with requirejs used like that.

[bgrins]

@fadomire can you check now with https://raw.github.com/bgrins/spectrum/master/spectrum.js? I added a call to window.tinycolor = tinycolor outside of the imported code, so it will fix the current issue and still avoid the issue with having to manually make changes every time TinyColor is imported.

[fadomire]

@bgrins i checked and it gives the same error as before.

But did you checked my previous comment ?

i found that assigning window.tinycolor in the requirejs callback makes it working as expected as shown here :
http://jsfiddle.net/efMmB/17/

[bgrins]

Yes I saw the workaround, but I'd like for users to not have to have extra code :). Can you check https://raw.github.com/bgrins/spectrum/master/spectrum.js now and see if it is working after just forcing it to be set to window? I still feel like there should be a way to handle this outside of the tinycolor modifications, but I'm OK with manually copying this change over if there isn't - as it doesn't change that often.

Could we directly call require after the initialization of tinycolor and use your code example to set it to window from within the plugin? Not sure the behavior of require("tinycolor") - if there is a way to call it without it doing an extra HTTP request.

[fadomire]

Your last version, where you changed tinycolor code and make it always window.tinycolor works.

But i think there is a misunderstanding about requirejs behavior.
In the "workaround" i found, there is no extra http request and not really "extra code".

What happens is requirejs load spectrum lib, and in his load callback we pass tinycolor object returned by the following code :

// AMD/requirejs: Define the module
else if (typeof define !== "undefined") {
    define(function () {return tinycolor;});
}

the only extra code is to assign tinycolor to global scope in the load callback :

require(['https://raw.github.com/bgrins/spectrum/master/spectrum.js'], function (tinycolor) {
  window.tinycolor = tinycolor;
  $("#colorpicker").spectrum({
    color: "#f00"
  });
});

I'm not sure what you mean by "Could we directly call require after the initialization of tinycolor".

IMO best options to fix this issue are :
leave the code like revision 5c2eaf6
and maybe add some precision in the Readme on how to use spectrum with requirejs. The code is just normal requirejs code, only assigning tinycolor to global scope is an extra requirement
OR
put tinycolor code out of spectrum (but you explained why you would prefer not to). the pro for that is mainly for people using tinycolor already as an independant lib, it would avoid them to have dupicate code.

Overriding tinycolor default code just feel wrong.

Sorry for the explanation if it was already clear :)

[bgrins]

Thanks for the explanation, I saw the extra require callback as an inconvenience to requirejs users, but it sounds like it may not be that big of a deal.

What I meant by a workaround without extra HTTP calls / code, is that I wanted to do what you are doing, only inside the library so users wouldn't have to. So something like:

(function() { 
    // tinycolor initialized as normal
});

require("tinycolor", function(tinycolor) {
    window.tinycolor = tinycolor;
});

$(function () {
    if ($.fn.spectrum.load) {
        $.fn.spectrum.processNativeColorInputs();
    }
});

Two issues here:

1. I have no idea if this code would actually work
2. Would this only improve the experience for requirejs users? What about yepnope or other script loaders?