Feature/amd support #124
Feature/amd support #124
Conversation
This is a hybrid implementation that allows skrollr to be used with both AMD and global namespace
Please read the note related to exposing skrollr through require config. This patch allows for hybrid usage as well (both AMD and global scope include). AMD version does not use DOMContentLoaded event - require.js reference should be put at the bottom of your page's body
|
First of: Thank you for taking the time and thanks for submitting a clean pull request which passes listing etc. But: As I said in #67 I'm not sure if I want this included. I don't see the benefits. I mean, require.js is twice as large as skrollr itself. And it's not like skrollr is used in big client side applications with dozens of dependencies, views, models, etc. And I don't see a use case were you want to load skrollr conditionally, either you need it or you don't. Feel free to argue with me. |
|
@Prinzhorn : The approach I am suggesting does not introduce any dependencies in itself. It makes skrollr more mindful to its execution environment. Some people (including myself) we'll be using skrollr in a project structured using AMD modules and I find it very useful to have support for require built in - especially given the fact that the impact on the codebase is tiny and it's also backwards compatible so existing code will continue to work. Furthermore, using things like Almond, one can easily cut the loader footprint down to 1kb. I would be happy to integrate this in a future pull request but I also feel like the size argument is pretty weak given that most people using the AMD support have already committed to require.js because of its organizational benefits and hence it's size becomes a static overhead spread across the whole project - not any library in particular. Feel free to argue with me :) |
|
You know what, looking at the source again there's actually a problem with your pull request: It will fail in IE, because the IE plugin expects a global |
|
@Prinzhorn : good call. let me see how this can be fixed |
|
@callmephilip Don't put too much effort into it now. I'm still not settled. There's the skrollr.menu plugin and there's a skrollr.path plugin coming. Those and everything that's coming would need to be made compatible. As I'm not using amd, I don't feel like this would work well. If I advertise "amd support" and it breaks, that's a bad thing. |
|
Understandable. I am interested in the amd version of the skrollr for my own projects so I might as well make sure it does not break your existing codebase. |
|
Sounds reasonable. Take a look at how to make these files compatible: https://github.com/Prinzhorn/skrollr/blob/master/src/plugins/skrollr.ie.js Now that I look at them again, there's another problem. Take a look at https://github.com/Prinzhorn/skrollr/blob/master/src/plugins/skrollr.menu.js#L109 . The script depends on being loaded with the document. I don't think there's any way to handle this with amd. |
|
I have managed to patch ie and menu plugins using similar hybrid solutions. You can see more details in the respectif commits in my fork of skrollr: here, here and here. Demos: Global namespace If at some point you have any interest in including this into skrollr, I'd be more than happy to put together a clean pull request. |
I haven't forgotten about this. I'm doing a lot of changes to the codebase right now, so just wait some weeks possibly months and I'll get back to you. |
|
We're using skrollr on a couple of pages in a big client side app. We've patched an older version of the script for AMD support to work with our build process. Just backing up @callmephilip 's point that people are using skrollr in AMD structured projects. Not making a case either way for AMD support out of the box. |
|
👍 for @callmephilip point |
|
+1 please! I know this might seem unnecessary if you're not using AMD currently, but it's cheap & easy enough that it seems to pass the "why not?" test. |
|
Will this be implemented? |
|
I agree with @Prinzhorn about not adding AMD, but for a different reason: AMD isn't the solution to module-loading on the web. I too once thought that AMD was the answer, but reading http://tomdale.net/2012/01/amd-is-not-the-answer/ and talking with the guys from component I've totally changed my mind about RJS. Using CommonJS is a much better alternative and requires significantly less "wrapping". Using something like https://npmjs.org/package/webmake, concatination is done as a build step, rather than loading all the modules one-by-one via HTTP. And CJS is already used by all Node.js packages - making it a popular, mature solution. Thus, I encourage you to avoid adding RJS support and use CJS instead. |
|
It seems worth posting the response from James Burke (the primary developer behind Require JS) to the article linked above: http://tagneto.blogspot.com/2012/01/reply-to-tom-on-amd.html I understand why one might chose not to use AMD in their application, but it doesn't seem like there's really all that much overhead involved in supporting it for a tool like skrollr — and then you've made it work much better for a whole group of developers. Also, it doesn't need to be a choice between AMD & CJS, check out UMD for standardized method of supporting both: |
|
+1 for AMD support! |
|
@Prinzhorn : as discussed last week, here's my first take on the AMD support. Open for questions/comments/suggestions: https://github.com/callmephilip/skrollr/compare/feature;amd |
|
Great, thanks for taking the time! Maybe already create a new PR for discussion. I'm not able to add line-comments that way. |
|
Closing in favor of #409 |
Here's my take on adding AMD support for skrollr. This approach supports module loading through require.js while keeping skrollr backwards compatible in case one wants to include it in a standard way through (script).
Demos: Amd version and Non Amd version