Skip to content
This repository has been archived by the owner on Apr 20, 2023. It is now read-only.

Add support for not waiting for the DOM to be ready #229

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

Conversation

jspath
Copy link

@jspath jspath commented Jun 4, 2013

Specifying a true value for "immediate" in the config makes it so whenReady immediately executes whatever it is passed.

I know this is not ideal and breaks the "golden rule".

However, we've run into instances where a customer's page takes a long time to be "ready" and therefore our application also loads slowly, which they then complain to us about.

We've been using this modification for many months now for all customers and have not had any issues.

@@ -146,7 +146,7 @@ easyXDM.stack.PostMessageTransport = function(config){
}
},
init: function(){
whenReady(pub.onDOMReady, pub);
whenReady(pub.onDOMReady, pub, config.immediate);
Copy link
Owner

Choose a reason for hiding this comment

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

It seems to me that if the transports need to be aware of this flag, and that no down-stream code cares about it, then the decision whether to run immediately or to wait for whenReady should also be made here.
This does mean some code duplication though..

@jspath
Copy link
Author

jspath commented Jul 17, 2013

Are you suggesting something like this?

        init: function(){
            if (config.immediate) {
              pub.onDOMReady.call(pub);
            } else {
              whenReady(pub.onDOMReady, pub);
            }
        }

@mattcasey
Copy link

This would be a useful feature for me, for example if a third party site does not fully load, then my tunnel will never open. But for my clients, I need to be sure that it will load on every page view 100% of the time. Rather than have to fork the library, it would be better if the option was available as a setting.

There seems to be concern that it's not best practice to create an iframe before DOMContentLoaded, but all that's technically required is document.body, which is the fallback unless the browser supports document.readyState, and I'd rather the library allow developers to decide how and when the tool should be implemented.

@sahanDissanayake
Copy link

whats happening with this guys ? Same issue with my app. My app creates half of the page o it is vital to load the app ASAP. or the users complain that the app is slow. Is this happening ?

@oyvindkinsey
Copy link
Owner

I'm hesitant to add this to the core library itself, when this is something that can be done by using the friendly-iframe architecture (https://www.facebook.com/note.php?note_id=10151176218703920).

@sahanDissanayake, would this work for you?

@sahanDissanayake
Copy link

Thanks Øyvind, So if I use FIF, there is no use of easyXDM ? FYI, im quite new to iframe usage. So still learning.

@oyvindkinsey
Copy link
Owner

No, FIF simply allows the page to create a new environment that doesn't block the top page, and where whenReady should be able to fire almost immediately.
It does involve a nested dynamically written iframe though, so unless you're comfortable with this, it might not be the right approach for you.

@sahanDissanayake
Copy link

sahanDissanayake commented Jun 24, 2016

Not really comfortable right now. I don't wanna break something thats working already. I will slowly move on to other solutions. easyXDM works ok for now.

Just to be clear, easyXDM does not block the page right ?

@akleiber
Copy link

Here is a very good explanation http://www.lognormal.com/blog/2012/12/12/the-script-loader-pattern/

We currently use FIF together with easyXDM, so it is working :-)

Copy link
Owner

@oyvindkinsey oyvindkinsey left a comment

Choose a reason for hiding this comment

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

No need for the use of .call here, unless onDOMReady is explicitly bound to something else, than pub, which should not be the case

@oyvindkinsey oyvindkinsey force-pushed the master branch 3 times, most recently from 4e46f32 to 2769b63 Compare April 8, 2019 17:33
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.

5 participants