External scripts are evaluated async

brauliobo commented 11 years ago

As external scripts are removed from the content and then evaluated by inserting them into head, they end up being evaluated async which causes somes dependent symbols not to work. See this example

<script src='external_script_with_function'/>
<script type='text/javascript'>
  function_from_external_script_above();
</script>

function_from_external_script_above() won't work!

Commenting the line obj.scripts = findAll(obj.contents, 'script[src]').remove() workarounded the issue.

I understand that a sync evaluation may hang the application, but as this example demonstrate it is necessary on some cases. I suggest a configuration to support both behaviours or support only the sync behaviour.

josh commented 11 years ago

Sorry, but there is no way to run "blocking" scripts as you can on an initial page load after the fact. Its just not possible with the way DOM fragment parsers work.

brauliobo commented 11 years ago

sorry @josh, can't understand your explanation.

what do you mean ' as you can on an initial page load after the fact'? could you please elaborate more?

shogun70 commented 11 years ago

@brauliobo are you sure that commenting out that line made the scripts execute sequentially? I would expect this to not actually execute the scripts at all.

brauliobo commented 11 years ago

@shogun70 yes, I am, because this will make jquery's html() call to see those scripts

shogun70 commented 11 years ago

@brauliobo: Thanks - I didn't know html() executed scripts.

I think your suggestion makes more sense than the current behavior. (But IMO anything that could be done with scripts in PJAX'd content would be better done a different way)

daGrevis commented 11 years ago

This “problem“ bothers me as well.

So I get a pjaxed request that contains HTML of the new page and some script[src] tags.

Scripts are not executed.

If I remove this from the source code:

    obj.scripts = findAll(obj.contents, 'script[src]').remove()
    obj.contents = obj.contents.not(obj.scripts)

...scripts are executed (well, alert(1) is executed correctly).

So what's the point of removing scripts from response?

I'm looking for a way to inject scripts when a page loads, don't want to load every script (script per page) in container.

Thanks!

daGrevis commented 11 years ago

To summarize, what's the point of removing scripts from HTML response?

lichunqiang commented 10 years ago

Wating the reason... :)

mislav commented 10 years ago

There are two types of <script> tags:

The ones with inline JavaScript in them. These are untouched by pjax but still get executed (eval'd) after the HTML inside the pjax container has been inserted via jQuery's .html() method.
The ones with src attribute (external scripts). We extract these from the pjax response and insert them into the <head> of the document after the new HTML has already been inserted into the pjax container. We only insert them in the document if there aren't already external scripts with the same src attribute, to prevent accidental repeated loading of the same external script.

If you think you've found a bug with this system please be more specific about what you're doing—which version of jquery-pjax are you using, what does your pjax response looks like, which script tag you expected to get executed but never did?

brauliobo commented 10 years ago

@mislav Please read the description of this issue. The problem is with the execution order, which is not preserved when using script and src.

mislav commented 10 years ago

Ah OK I understand. This is another example of how pjax doesn't (and can't) behave identically to static pages. You should construct your external script tags to not depend on each other at eval time, or handle script loading manually in your app using pjax events.

TL;DR; due to browser limitations, it's nontrivial for us to fix this without implementing a whole script loader withing pjax plugin.

brauliobo commented 10 years ago

I had proposed a way that is already handled by jquery automatically. jquery knows how to deal with script tags on the ajax response.

mislav commented 10 years ago

@brauliobo What does jquery do?

brauliobo commented 10 years ago

http://api.jquery.com/html/ and http://api.jquery.com/append/ handles script on the html they receive. Basically, it calls jQuery.globalEval() on inline scripts and jQuery.getScript() on external scripts (the ones with src attribute). I could not yet find documentation on this behaviour, I know it because I looked to jQuery's code.

That's why just commenting the line where pjax extract script tags fixed the problem, because later jQuery handled them.

ghost commented 9 years ago

The situation that @brauliobo (obrigado por apontar a linha culpada) describe is really common. This issue has more than one year and the solution is really easy. No need to implement anything. At least add something about this to the doc/wiki.

mislav commented 9 years ago

OK, let's clear something up first. Or, you people are welcome to prove me wrong.

Consider this HTML:

<p>Hello world!</p>
<script src="external1.js"></script>
<script src="external2.js"></script>
<script>External.something("foo")</script>

Here we have multiple external script loads, and an inline script that relies on the example External module loaded from those external scripts. Now, here's what I think I know about script loading:

When this HTML loads normally as part of a web page, each script[src] tag will block execution of everything after it, basically holding the page frozen until it finishes loading and executes the loaded script. Multiple external scripts are guaranteed to execute in DOM order. Then the inline script will run and everything will work.
When this HTML is instead inserted via jQuery's html(), jQuery will use getScript() to fetch and execute the script[src] contents. However, because this process is implemented with XHR and therefore is asynchronous, multiple scripts are not guaranteed to execute in DOM order, and they are not guaranteed to execute before the inline script (which jQuery evals). Since jQuery doesn't have any of this documented, this is just my guess. This is your chance to look into this and correct me.
When this HTML is instead inserted via pjax, we extract all script[src] and put them in HEAD so they automatically load and execute in async fashion. This is an alternative to dynamic script loading that we prefer because it doesn't rely on eval. However, the order of script execution is stil not guaranteed, and inline scripts are going to execute before the external scripts. This is an unfortunate side-effect and limitation of pjaxing your site, and is fixed by not having inline scripts that rely on external script execution blocking and order.

Now, nobody other than me would be happier if we managed to fix this and ensure that pjaxed sites work always exactly the same as native page loads. Because of that, I'm accepting constructive suggestions on how we can technically make this work. However, the solution can't depend on eval because we want pjax to be compatible with sites that use Content Security Policy to disable eval for safety reasons. One such site is GitHub.com, where we use pjax ourselves. Therefore, the solution can't rely on jQuery.getScript().

brauliobo commented 9 years ago

@mislav 2 is wrong. jQuery does guarantee execution order. I'm not sure, but I think it runs scripts syncronously when found by html()

mislav commented 9 years ago

@brauliobo OK, that might be true. Can you point us to somewhere where we can verify this? Perhaps a location in its source code

brauliobo commented 9 years ago

See _evalUrl and domManip from https://github.com/rails/jquery-rails/blob/master/vendor/assets/javascripts/jquery2.js#L5408 and https://github.com/rails/jquery-rails/blob/master/vendor/assets/javascripts/jquery2.js#L8316

brauliobo commented 9 years ago

In _evalUrl you see async: false

brauliobo commented 9 years ago

jQuery._evalUrl is a variable and can be replaced by the user.

mislav commented 9 years ago

Ah so it doesn't use getScript() exactly. Thanks for pointing that out.

OK so what we can do in pjax is try to detect whether eval is allowed in the current execution environment. If it's allowed, we can not extract script[src] and let jQuery do its job. If eval is not allowed by CSP, like on GitHub.com, we'll keep extracting all script[src] and put them into HEAD.

@josh How does that sound?

josh commented 9 years ago

Its not really possible in CSP 1.0 to feature detect if eval() is allowed in a sane way.

mislav commented 9 years ago

No? What happens if you call eval("1+2")

josh commented 9 years ago

It will crash and send back a CSP violation ping back to our servers each time. On Tue, Dec 9, 2014 at 9:51 PM Mislav Marohnić notifications@github.com wrote:

No? What happens if you call eval("1+2")

— Reply to this email directly or view it on GitHub https://github.com/defunkt/jquery-pjax/issues/331#issuecomment-66407781.

brauliobo commented 9 years ago

I haven't able to fully test this but it seems promising:

function cspEnabled() {
  try {
    eval("return false;");
  } catch (e) {
    return true;
  }
}

from http://stackoverflow.com/questions/19140169/how-to-detect-content-security-policy-csp

brauliobo commented 9 years ago

it is pretty similiar to what angular.js is doing: https://github.com/IgorMinar/angular.js/commit/e05b78a781c0c9007351d12c8f9fbd369d25fafb

https://github.com/angular/angular.js/issues/8162

brauliobo commented 9 years ago

https://github.com/angular/angular.js/blob/master/src/Angular.js#L869

ghost commented 9 years ago

@josh from CSP 1.0

If 'unsafe-eval' is not in allowed script sources:

Instead of evaluating their arguments, both operator eval and function eval must throw a security exception. [ECMA-262]

and with the code example by @brauliobo, it should work.

josh commented 9 years ago

Again, that causes a violation report which is unacceptable. On Wed, Dec 10, 2014 at 3:03 AM Leonardo Tietböhl notifications@github.com wrote:

@josh https://github.com/josh from CSP 1.0

If 'unsafe-eval' is not in allowed script sources:

Instead of evaluating their arguments, both operator eval and function eval must throw a security exception. [ECMA-262]

— Reply to this email directly or view it on GitHub https://github.com/defunkt/jquery-pjax/issues/331#issuecomment-66436099.

brauliobo commented 9 years ago

@josh So a CSP disable/enable option is the only alternative. (just like angular.js did)

josh commented 9 years ago

I don't really like the general idea of having two different modes that behave pretty differently. Not appending to head means that pjax can't track scripts it's eval'd in a consistent way. On Wed, Dec 10, 2014 at 10:07 AM Bráulio Bhavamitra < notifications@github.com> wrote:

@josh https://github.com/josh So an CSP disable/enable option is the only alternative. (just like angular.js did)

— Reply to this email directly or view it on GitHub https://github.com/defunkt/jquery-pjax/issues/331#issuecomment-66495338.

brauliobo commented 9 years ago

@josh Tracking scripts should be as easy as storing the list of src in a variable on pjax's context.

mislav commented 9 years ago

On Wed, Dec 10, 2014 at 10:06 AM, Bráulio Bhavamitra < notifications@github.com> wrote:

So an CSP disable/enable option is the only alternative.

It seems so, which sucks a lot because I don't want that to be an option. Developers of web apps would have to remember to change pjax configuration when transitioning to CSP, and we would have to maintain separate code paths.

I'd rather just draw a line and say we as a library won't support sites that rely on inline scripts and blocking, ordered external scripts load from pjax-loaded page bodies.

nkovacs commented 9 years ago

This commit uses onload to load the scripts serially, in the original order (excluding already loaded scripts). It also loads inline scripts in the original order, by appending them to the container (so it only relies on eval if you have inline scripts, like pjax does now). You do lose parallel script downloads, but this is necessary for correctness. It's still async though. I've only tested this on Chrome, and I forked Yii's version of pjax, because I needed it for Yii 2.

mislav commented 9 years ago

This commit uses onload to load the scripts serially, in the original order (excluding already loaded scripts). It also loads inline scripts in the original order, by appending them to the container (so it only relies on eval if you have inline scripts, like pjax does now).

I looked at the implementation and it seems solid. We might look into including something like that if we can simplify the implementation, and if the <script> tag onload events are reliable cross-browser. However right now I would much rather introduce a configuration option that lets you switch between jQuery mode (sequential eval) and current async mode (modern, CSP-enabled websites).

brauliobo commented 9 years ago

@mislav +1

dmill-bz commented 9 years ago

@mislav +1 on at least having a configuration option for this. Has there been any progress with this since?

mislav commented 9 years ago

No progress since. I wasn't super motivated to prioritize this since pjax works very well for our use case and we generally don't need loading in sync.

arobbins commented 8 years ago

Are there still plans to follow through with this feature?

staabm commented 8 years ago

Couldnt code like quoted in the inital report use the onload event to intialise properly?

<script src='external_script_with_function.js' onload='function_from_external_script();'/>

nkovacs commented 8 years ago

That's not a solution if you have an existing site you'd like to pjaxify and especially if the code is out of your control, because it's part of a framework, like Yii 2.

RichardTMiles commented 7 years ago

The filament group has a repository called loadJS : https://github.com/filamentgroup/loadJS/blob/master/loadJS.js

<script>
/*! loadJS: load a JS file asynchronously. [c]2014 @scottjehl, Filament Group, Inc. (Based on http://goo.gl/REQGQ by Paul Irish). Licensed MIT */
(function( w ){
    var loadJS = function( src, cb ){
        "use strict";
        var ref = w.document.getElementsByTagName( "script" )[ 0 ];
        var script = w.document.createElement( "script" );
        script.src = src;
        script.async = true;
        ref.parentNode.insertBefore( script, ref );
        if (cb && typeof(cb) === "function") {
            script.onload = cb;
        }
        return script;
    };
    // commonjs
    if( typeof module !== "undefined" ){
        module.exports = loadJS;
    }
    else {
        w.loadJS = loadJS;
    }
}( typeof global !== "undefined" ? global : this ));

 $(document).on('pjax:end', function () {
             $(document).on('submit', 'form[data-pjax]', function (event) {
                        $.pjax.submit(event, '#ajax-content')
             });
              loadJS("components/jquery/jquery.min.js' ) ?>", function () {
                      loadJS("bower_components/bootstrap-datepicker/js/bootstrap-datepicker.js", function (){
                             $('#datepicker').datepicker({autoclose: true});
                    });
            });
 });
</script>

Is this a possible solution?

defunkt / jquery-pjax

External scripts are evaluated async #331