bgrins / TinyColor

Fast, small color manipulation and conversion for JavaScript
https://bgrins.github.io/TinyColor/
MIT License
5.08k stars 438 forks source link

window.tinycolor is not defined if JS loader is used #122

Closed olee closed 8 years ago

olee commented 8 years ago

When a loader like SystemJS or RequireJS is found, tinycolor skips registering the window.tinycolor variable. This breaks other modules that don't use a loader and depend on window.tinycolor.

bgrins commented 8 years ago

I thought it was using a pretty standard setup for module registration. Do you have an example where this doesn't happen?

olee commented 8 years ago

I was trying to use https://github.com/brianpkelley/md-color-picker which needs tinycolor with typescript. md-color-picker relies on tinycolor being defined in window, which was missing because SystemJS was active.

bgrins commented 8 years ago

I guess I'm not sure of how to distinguish between 'a module loader is present' and 'this script is being loaded with a module loader'. I'm not sure I've seen UMD use both define or module.exports and also setting to 'window'

olee commented 8 years ago

Just remove the else and always add tinycolor to window. It should not skip that step even if a loader is present. JQuery for example does it the same way.

bgrins commented 8 years ago

Just remove the else and always add tinycolor to window.

But what about in the node js environment?

olee commented 8 years ago

Check if window is defined? :smirk: I don't see any problem with that. At least I don't see any other good solution to the problem which isn't a hack....

bgrins commented 8 years ago

I imagine there's a case where someone is using a module loader because they don't want libraries to pollute the global namespace. I think what it's doing now is the 'standard' way to register modules with UMD. Are you able to load tinycolor via SystemJS and then set window.tinycolor after that finishes loading?

jt3k commented 8 years ago

yes if u need link to tinycolor in property of window object then:

require(['tinycolor'], function(tinycolor){
  window.tonycolor = tonycolor;
});

this is the right way

olee commented 8 years ago

Well - I solved in kinda that way for now, too.

I imagine there's a case where someone is using a module loader because they don't want libraries to pollute the global namespace.

Yes, but not quite. It's more to prevent collisions than just populating the window namespace. If a loader is used like it should be, a window namespace with many properties will not cause any problems, but can still help to solve backwards compatibility issues with libraries that don't support module loading yet. That's why I suggested this.

TL;DR: Can you find any negative effect of registering tinycolor in the window namespace? If you can think of one then of course, it would be better to leave it as this and do by with other solutions like the one mentioned, but I at least can't think of one.

jt3k commented 8 years ago

@olee Imagine you have legacy code which uses a global variable with the name tinycolor...

Way with require more flexible.

bgrins commented 8 years ago

Agreed with @jt3k and https://github.com/bgrins/TinyColor/issues/122#issuecomment-196693319 is a workable solution to the problem. Closing this out.