Support AMD

[ethanresnick]

Addresses the issue mentioned in #31 by putting the function that needs to run immediately (but doesn't depend on skrollr) outside the module definition.

[Prinzhorn]

The only way to prevent this, I think, would be to put this code in the HTML directly (making sure it's blocking but not taking an extra request) rather than use AMD. A bit annoying, but probably the best solution.

Are you talking about the same thing I mentioned in #31?

Maybe we should exclude it and instead document it (people would need to include it manually in their code). It needs to be run ASAP, I think its too late when it got fetched by the loader.

[ethanresnick]

Yep, we're talking about the same thing. On Apr 30, 2014 4:12 AM, "Alexander Prinzhorn" [email protected] wrote:

The only way to prevent this, I think, would be to put this code in the HTML directly (making sure it's blocking but not taking an extra request) rather than use AMD. A bit annoying, but probably the best solution.

Are you talking about the same thing I mentioned in #31https://github.com/Prinzhorn/skrollr-menu/issues/31 ?

Maybe we should exclude it and instead document it (people would need to include it manually in their code). It needs to be run ASAP, I think its too late when it got fetched by the loader.

— Reply to this email directly or view it on GitHubhttps://github.com/Prinzhorn/skrollr-menu/pull/41#issuecomment-41770993 .

[stianlik]

I think this is a good solution. For cases where AMD is not used, the code will work exactly as before. When AMD is in use we simply need to be aware that the module should to be loaded as early as possible. I tested the patch using a similar setup as the example below, loads fine into an AMD environment.

For issue #31

require([ 'skrollr', 'skrollr-menu' ], function(skrollr, skrollrMenu) {
    // app init
})

is equivalent to

<script src="skrollr.js">
<script src="skrollr-menu.js">
<script>
// app init
</script>

and both cases result in

1. skrollr is loaded
2. skrollr-menu is loaded and initialization code is run
3. app init is executed

In any case this is better than the current situation, where the latest version of skrollr cause skrollr-menu to fail if loaded into an AMD environment (var skrollr = window.skrollr; is undefined).

To be completely sure that the document is scrolled to top on load, it should be sufficient to inform users about the issue and posting a code example of how to solve it. It could actually be reasonable to remove the scrollTo-code completely so that users can decide how the initial load should be performed.

[Prinzhorn]

Sorry for taking so long.

It could actually be reasonable to remove the scrollTo-code completely so that users can decide how the initial load should be performed.

It is absolutely mandatory that this code is executed, otherwise mobile support is completely broken.

[cmwelsh]

This plugin is still broken if you load skrollr with AMD.

I hacked around this by setting window.skrollr variable manually.

        window.skrollr = skrollr;
        define('skrollr', function () {
            return skrollr;
        });

AMD users are used to browser plugins leaking global variables... just release a new skrollr with the above change IMHO.

[Prinzhorn]

All these discussions are the reason why the original pull request for adding AMD to skrollr was open forever. I don't use AMD, I never wanted it, but I eventually accepted the pull request in favor of the community. I won't actively push the AMD integration forward, but I will accept pull requests if they are reasonably documented and don't contain any obvious loopholes like this one does.