Skip to content

Use own $ variable, make sure Jquery can be loaded#228

Merged
Sebobo merged 1 commit intogermanysbestkeptsecret:masterfrom
falconshark:master
Jan 10, 2017
Merged

Use own $ variable, make sure Jquery can be loaded#228
Sebobo merged 1 commit intogermanysbestkeptsecret:masterfrom
falconshark:master

Conversation

@falconshark
Copy link
Contributor

If use $ variable directly, Wookmark may be cannot call Jquery function (Such as running on Drupal).

@yookoala
Copy link

yookoala commented Jan 9, 2017

I have same issue using the library with Drupal. Drupal use jQuery with noConflict mode to avoid conflicting use of the $ variable. Therefore jQuery plugin that uses $ alias directly will have problem.

As changed by this PR, it is a common practice to use immediately-invoked function expression to protect $ alias in your scope. It is also cited in the official learning doc.

Hope this PR is accepted.

@Sebobo
Copy link
Member

Sebobo commented Jan 9, 2017

Hey, thanks for the PR!
Of course this has to be fixed, I overlooked that because I test on the existence of jQuery first. But I'm not sure if the closure is the best idea.
Then the plugin looks more like a jQuery plugin which it actually isn't necessarily.
Instead the ~3 places where jQuery is used could just be changed to use jQuery instead of $.

What do you guys think?

@Sebobo Sebobo self-requested a review January 9, 2017 12:38
@Sebobo Sebobo added the Bug label Jan 9, 2017
@yookoala
Copy link

Immediate-invoked function expression binds the jQuery variable when the script is called. Whereas if you changed all $ to jQuery, the script will simply call the current window.jQuery object.

In that sense, immediate-invoked function is much more friendly to jQuery users when they need to use multiple jQuery versions together. They simply have to arrange the script call order properly like this:

...
<script src="js/jquery-3.1.1.js" />
<script src="js/Wookmark.js" />
<script src="js/jquery-1.17.1.min.js" />
<script src="js/some-old-plugin.js" />
...

So as long as Wookmark is depending on jQuery, I think adopting the pattern is a better idea than replacing all $ with jQuery.

@Sebobo Sebobo merged commit ef96eb3 into germanysbestkeptsecret:master Jan 10, 2017
@Sebobo
Copy link
Member

Sebobo commented Jan 10, 2017

@yookoala true, I never do that, but I know there are websites which need it.
I'll try to make a new release in the next days.

@falconshark
Copy link
Contributor Author

@yookoala @Sebobo Thank you very much! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants