Two minor changes

[asppsa]

• Deleted an if-statement that wasn't needed (as the statement was nested inside an identical "if" at line 2278)
• "Fixed" the AMD import. I think I did this correctly, but you may disagree. The define function is supposed to have as its second argument a function that will be called, passing in the module's dependencies. I guess another way to do this would be define([], function() { return factory(plugin); });, if it is necessary to pass that plugin variable in still.

[skeptic35]

@asppsa actually what you describe as "another way" would be the correct way.
The way it is now, the factory function gets called as soon as Selectr is loaded, meaning the Selectr constructor gets called without any arguments and throws the following error: "You must supply either a HTMLSelectElement or a CSS3 selector string." AMD loading is thus effectively broken.

Passing in the plugin parameter is imho completely unneccessary, as it is never used in the factory function. Also the first parameter to define can be ommitted, as there are no dependencies being passed in.
So to sum it up, the code should look like this:

    if (typeof define === "function" && define.amd) {
        define(function(){return factory()});

[asppsa]

@skeptic35, are you saying that the fix in 73eed26 generates the error you describe? It seems to be working for me, although I'm JSPM rather than RequireJS.

[skeptic35]

@asppsa yup - that's what I'm saying. Don't know why it is working for you because it actually doesn't have anything to do with RequireJS, it's just how javascript works.
This:

define([], factory(plugin));

calls the factory function and passes it's return to the define, while this

define([], factory);

doesn't call the function and instead passes the function itself to define, which is exactly what we want here.

I have to apologize though for the suggested solution. Was in a bit of a hurry, so I didn't really think this through.
As there are absolutely no dependencies being passed in there is also abosolutely no need for the function returning the factory. Meaning this here would be much more easy and would also work:

define(factory);

In short: Just make sure the factory function doesn't get called and we're fine ;)

[skeptic35]

P.S. - come to think of it - maybe it does have to do with using RequireJS. Dunno how JSPM works, but RequireJS calls the function passed to define as soon as the according file is required.

[asppsa]

@skeptic35, my PR changed line 11 from define([], factory(plugin)); to define([], factory);. Isn't this what you are saying should work?

[skeptic35]

Oops - you're right - it did. That's kind of embarassing - sorry.
See, the thing is that I installed Selectr just yesterday via npm. I saw it throw that error, saw the define([], factory(plugin) in the code, saw your PR and very obviously drew totally wrong conclusions without really checking which way round your PR was.
Please accept my apologies.
Now I just have to find out why the version I installed yesterday says it's a 2.4.0 (same as the current version) but still contains the bug you fixed...

[asppsa]

No need to apologise! I was definitely not sure about my patch, so I am glad someone using requirejs can test things out.

I think the issue is that there has not been a new release since the PR was merged. You could perhaps try installing from git instead of and see if that works.

[skeptic35]

You could perhaps try installing from git instead of and see if that works.

I kinda did that already ;) - works