mobify / pinny

A mobile-first content fly-in UI plugin
MIT License
23 stars 4 forks source link

Fix 49: Create standalone pinny #64

Closed hcstephencheung closed 8 years ago

hcstephencheung commented 9 years ago

Attempt to fix https://github.com/mobify/pinny/issues/49

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

scalvert commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

@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).

scalvert commented 9 years ago

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 commented 9 years ago

@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/

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

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 commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

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

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

scalvert commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

@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 commented 8 years ago

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!