yiisoft / jquery-pjax

pushState + ajax = pjax
http://pjax.herokuapp.com
MIT License
143 stars 40 forks source link

Fixed loading of scripts in pjax containers #30

Closed SilverFire closed 8 years ago

SilverFire commented 8 years ago

Closes yiisoft/yii2#3680 Closes yiisoft/yii2#4814 Closes yiisoft/yii2#8540 Closes yiisoft/yii2#8916 Closes yiisoft/yii2#8702 Closes #23 Closes #22

This patch was proposed by @nkovacs. I've tested it for 2 week in my project and found no problems with it.

May be merged if nobody complains.

nkovacs commented 8 years ago

Instead of using onload/onerror, you could also use an ajax call and jquery's globalEval, like jquery._evalUrl, except with an async ajax call and the callbacks. Btw, there's a proposed patch to fix it, but the original pull request is broken, there's a better pseudo-implementation in the comments: https://github.com/jquery/jquery/pull/2126

SilverFire commented 8 years ago

@nkovacs did you test it?

nkovacs commented 8 years ago

Not with this, no. I monkey patched $.domManip, and it works. This is how globalEval works:

if ( code.indexOf("use strict") === 1 ) {
    script = document.createElement("script");
    script.text = code;
    document.head.appendChild( script ).parentNode.removeChild( script );
} else {
    indirect( code );
}

I'm not sure document.head.appendChild is guaranteed to be synchronous, maybe not, but in that case it's strange that jquery uses it. The else branch uses eval, so that's synchronous, and in my case the code doesn't have 'use strict' (yii's activeform and validation js), so it works for me.

The onload thing might not work on old IE, but since pjax only works on browsers supporting history api, that's not an issue.

SilverFire commented 8 years ago

But your implementation in this PR works pretty good, isn't it? Do you think it must be changed?

nkovacs commented 8 years ago

I don't know which one is better. Onload works in practice on the modern browser I tried, not sure how reliable it is. Eval should be good, but it's unsafe, and not allowed in many places.

SilverFire commented 8 years ago

Okay, I'm preferring implementation in this PR as it is tested

mikehaertl commented 8 years ago

@SilverFire What about the issue in yiisoft/yii2#6655? Shouldn't it also try to address this problem: https://github.com/yiisoft/yii2/issues/6655#issuecomment-149196252?

SilverFire commented 8 years ago

@mikehaertl yes, your idea makes sense. Will create a PR in yii2 repo later

mikehaertl commented 8 years ago

@SilverFire So just to be clear: you want to unify the script filter mechanism, right? Because to me that's the root of many problems: Pjax and Yii both use their own way to make sure, that a script is not loaded twice. But these two ways are not compatible.

So maybe we should refactor Yii's script filter and use the same mechanism like here. Ideally we should even reuse the same code...