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.

Sign up for GitHub

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

Jump to bottom

that scripts are moved to head, will lead to a fetal erorr with inline script depends on the script files. #478

Closed
dallaslu opened this issue Feb 4, 2015 · 7 comments
Closed

that scripts are moved to head, will lead to a fetal erorr with inline script depends on the script files. #478

dallaslu opened this issue Feb 4, 2015 · 7 comments

Comments

@dallaslu
Copy link

dallaslu commented Feb 4, 2015

the pjax response content:

<script src="/js/test.js">
</script>
<script>
    alert(Test.test());
</script>

the test.js:

var Test = {
    test: function(){
    return "Testing!";
    }
}

At first, Pjax will parse html content and run the inline script with 'eval', but the test.js hasn't been load because it is removed from the content and will be added to head later. The console prints 'Test' is undefined, and so, test.js will be never added to head, that means the object Test will always be undefined.

Script tags with src attribute are not all javascript libs only with object defined in it, they could contain some lines which is executable, and should run when loaded every time for the pjax response content.

Moving all scripts with src attribute to head is a kind of idiot behavior.
@dallaslu dallaslu mentioned this issue Feb 4, 2015
Closed
@mislav
Copy link
Collaborator

mislav commented Feb 4, 2015

Yes, pjax breaks this use case. Read #331 (comment) and subsequent comments for more information. However, we can't fix this within pjax completely for all sites out there because the browsers don't let us execute dynamically inserted scripts in order if Content Security Policy is turned on and eval is forbidden. This is the primary reason we're moving all script[src] to <head> and not supporting other inline scripts.

We would welcome your suggestion for a solution that:

  1. Is not "idiot" by your standards; and
  2. that also works on CSP-enabled sites such as GitHub.com.

@dallaslu
Copy link
Author

dallaslu commented Feb 4, 2015

Thank you for your prompt reply and patiently explained. First, this method does not work in many cases. It wasted a lot of my time to the debugger. Prior to finding the perfect solution, and it should be removed off, or optional, according to the properties such as script tags to decide whether to move to the head.

I agree that 'construct your external script tags to not depend on each other at eval time', but can't agree that 'handle script loading manually in your app using pjax events'. Pjax should be a non-invasive improvement, it shouldn't need a lot lines of code to fix the bugs caused by the immature method executeScriptTags.

@dallaslu dallaslu mentioned this issue Feb 5, 2015
Open
@mislav
Copy link
Collaborator

mislav commented Mar 3, 2015

Closing in favor of #480 and as a dup of #331

@mislav mislav closed this as completed Mar 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mislav @dallaslu and others