Fix 49: Create standalone pinny

[hcstephencheung]

Attempt to fix #49

Added custom shim configuration scripts for jQuery and Velocity so that pinny.js can be a standalone script.

[scalvert]

Hey @hcstephencheung! Thanks for the PR. Can you give us a bit more detail on the problem this is trying to solve? As it stands, both Velocity and jQuery are already AMD compatible, so I don't think we need those wrappers. Perhaps I misunderstand what this PR is for.

[hcstephencheung]

@scalvert There was an issue trying to compile pinny with jQuery/Velocity excluded in the build. The custom shims were aimed to fix those compilation errors. @tedtate wanted to create a standalone pinny so that users can install pinny with their own versions of jQuery/Velocity.

[tedtate]

@scalvert This is trying to make it easier for Non-AMD users to use Pinny. Right now you would need to include something like ~8 scripts to make pinny work. This PR moves that number down to 2 (jQuery and Velocity).

[highruned]

This should probably be global.jQuery, to be in line with the window.jQuery below (support for noConflict).

Actually, why not return ret || global.jQuery || global.Zepto, removing the && define.amd.jQuery check, and rename this file to selector_library_wrapper.js?

[tedtate]

+1 on all of @ericmuyser 's suggestions.

I actually don't think we even really need to check whether define exists because we are only including this file inside a closure which includes define. Same would go for the velocity wrapper.

[scalvert]

So help me understand. You're saying r.js needs this to build a standalone file? Also, jQuery and Velocity are both AMD compatible, so why do we need these wrappers?

[hcstephencheung]

@scalvert I can answer the first question, but you might want to consult @tedtate for the second question.

From my understanding, we are trying to find a way to define $ and Velocity in Require.js such that we are promising the compiler that $ and Velocity are defined (so other modules with those dependencies can still work), but they are actually externally included through a script tag.

The original idea was to find a way to do this in the config. The usual configurations to include jQuery would be including jQuery in the path, or adding a shim config on plugins that depend on jQuery. However, both methods result in jQuery being inlined in the build, which isn't what we wanted. The other solution is to include the define methods ourselves, which is what this commit is about.

Resource on how to include jQuery in Require.js separately during the build: http://gregfranko.com/blog/registering-the-jqueryui-widget-factory-as-an-amd-module/

• look at section "Common Questions"

Resource on how to include jQuery in Require.js normally:
https://github.com/jrburke/require-jquery

• "In particular, if using "shim" config you must build in jquery.js into your built file."

If there's a better way to do this, please do comment here. I'm still quite new at this, so I might not have a full understanding on the sophisticated r.js.

[highruned]

While jQuery and Velocity are AMD compatible, they don't necessarily expose the same module names we expect ($ and velocity). In the case of jQuery, it exposes jquery. In the case of Velocity, it exposes RELATIVE_PATH/velocity (from what I can see). That's the point, right?

[tedtate]

@ericmuyser Yup, that is the point. The way require handles module names is real stupid.

@scalvert The crux of the problem is two-fold from what I can tell:

1. The name that jQuery exposes via AMD isn't the one that we use.
2. If jQuery isn't included inside our built file the shim config option doesn't work.

I think this PR addresses both of those concerns but I'm also unsure if it is the most elegant way to do so with require. If anyone has better require foo than @hcstephencheung and I please feel free to chime in. Perhaps @donnielrt or even @noahadams

[donnielrt]

@tedtate could we not use require.js' map for this?

http://requirejs.org/docs/jquery.html#noconflictmap

[scalvert]

@tedtate @hcstephencheung OK thanks for explaining. It makes sense.

@donnielrt may be onto something there though - using map may work, as from my understanding it's for mapping names to a different alias.

[tedtate]

I think that is going to run into the same issue. You aren't going to be able to use map because we aren't actually including jQuery inside of our build.

Do you have time to verify if map will help us out @hcstephencheung?

[hcstephencheung]

@donnielrt it seems that even with map, we'll need to map the jquery to our own defined $ module (which is what the wrapper module is doing). Also, Zepto isn't AMD compatible so we'll still need to properly define $ to use whatever is available in global (assuming users are still free to use jQuery or Zepto)

@tedtate I can't really think of another solution to achieve the 2 problems you stated above, so perhaps this is the best solution to go with?

@scalvert other thoughts?

[donnielrt]

Hey, we’re looking to prune older unattended PRs. If this PR is still relevant and you would like to see it merged in, please reopen the PR and we’ll add it to our backlog! Thanks!