Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed loading of scripts in pjax containers #30

Merged
merged 2 commits into from
Nov 23, 2015

Conversation

SilverFire
Copy link
Member

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

nkovacs commented Nov 20, 2015

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: jquery/jquery#2126

@SilverFire
Copy link
Member Author

@nkovacs did you test it?

@nkovacs
Copy link

nkovacs commented Nov 20, 2015

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

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

@nkovacs
Copy link

nkovacs commented Nov 20, 2015

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

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

@mikehaertl
Copy link

@SilverFire What about the issue in yiisoft/yii2#6655? Shouldn't it also try to address this problem: yiisoft/yii2#6655 (comment)?

@SilverFire
Copy link
Member Author

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

@mikehaertl
Copy link

@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...

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

Successfully merging this pull request may close these issues.

3 participants