sciactive / pnotify

Beautiful JavaScript notifications with Web Notifications support.
https://sciactive.com/pnotify/
Apache License 2.0
3.65k stars 513 forks source link

New AMD define sequence breaks SystemJS loader (and thus JSPM) #206

Closed pfurini closed 8 years ago

pfurini commented 8 years ago

Hi, I'm using PNotify in a JSPM based project, and the latest version breaks the AMD compatibility layer of SystemJS (that JSPM itself use to load dependencies).

The issue reside in the first define call

define('pnotify-root', function(){
            return root;
        });

SystemJS seems unable to track this on-the-fly dependency in his internal registry, thus the next define call fails to resolve it properly. Removing the previous block of code, and replacing the main define with the following code restore SystemJS compatibility.

define('pnotify', ['jquery'], function(jq) { return factory(jq, root) });

Another option could be to try using the CommonJS format with SystemJS, but all my other external libraries are AMD and I'd prefer to stick with it. Anyway, that pnotify-root define is only syntactic sugar and, at least IMHO, removing it shouldn't harm anyone.

Thanks!

hperrin commented 8 years ago

Thanks for your help. :D