Skip to content
This repository has been archived by the owner on Jan 24, 2018. It is now read-only.

Support AMD #41

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Support AMD #41

wants to merge 3 commits into from

Conversation

ethanresnick
Copy link

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.

//In case the page was opened with a hash, prevent jumping to it.
//This code runs immediately, before skrollr may have loaded (if AMD's in use).
//http://stackoverflow.com/questions/3659072/jquery-disable-anchor-jump-when-loading-a-page
window.setTimeout(function() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this code executed once I do require(['skrollr-menu'], function() {}) (which means too late)?

By the way, where is the name of this module specified? I know nothing about AMD, but looking at Prinzhorn/skrollr#409 I can see that it's defined as skrollr. I mean, how do you required the menu plugin?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it's taken me a bit to get back to this.

When this code runs will depend on the require configuration, but there's no reason to assume it will generally run later than the non-AMD setup we have now. In the non-AMD version, it runs only after skrollr has been downloaded and executed (since it must be included before skrollr-menu), and then only after skrollr-menu's been downloaded. In the AMD version, the code will run after the AMD loader's been downloaded and executed, and then after the skrollr-menu module's been downloaded. But it won't have to wait for the skrollr module to donwload (AMD loaders download the resources in parallel) or for the skrollr-menu module to start executing (since this code is defined outside the module body).

Perhaps the bigger issue, though, is that the AMD loader won't block the page's rendering. So even if the browser runs the code at about the same time in either case, it may have already shown the rendered page to the user under the AMD version, causing a visible jump as the code scrolls back to the top. 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.

Re the module's name: it actually doesn't need one, and not naming it arguably makes it a bit more flexible. Only the skrollr module needs a hardcoded name, so that the plugins can dependably require it. With the setup in this commit, someone would require skrollr-menu like this:

require.config({
  //many config options go here
  //...
  paths: {
    "myNameForSkrollrMenu": "path/to/skrollr/menu"
  }
});
define(["myNameForSkrollrMenu"], function(skrollrMenu) {
   //my module that depends on skrollr-menu
}

Still, we can give it a name if you want.

@Prinzhorn
Copy link
Owner

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
Copy link
Author

Yep, we're talking about the same thing.
On Apr 30, 2014 4:12 AM, "Alexander Prinzhorn" notifications@github.com
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 #31#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//pull/41#issuecomment-41770993
.

@stianlik
Copy link
Contributor

stianlik commented May 5, 2014

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
Copy link
Owner

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
Copy link

cmwelsh commented Jul 1, 2014

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
Copy link
Owner

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants