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

Load scripts synchronously #85

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Kronuz
Copy link

@Kronuz Kronuz commented Aug 10, 2014

External scripts were being asynchronously loaded as part of the new page being loaded. This could break inlined scripts that depended on such scripts.

Example:

<script src="http://example.com/external_methods.js"></script>
<script>external_method();</script>

This pull request fixes this problem by waiting for scripts to load before inserting the remaining script elements.

External scripts were being asynchronously loaded as part of the new page being loaded. This could break inlined scripts that depended on such scripts.
@yazeed
Copy link

yazeed commented Aug 10, 2014

Please merge this, it's very critical.

vsyaco pushed a commit to vsyaco/trueinstantclick that referenced this pull request Aug 25, 2014
External scripts were being asynchronously loaded as part of the new
page being loaded. This could break inlined scripts that depended on
such scripts.

See original dieulot/instantclick#85 for
details
@dieulot dieulot added the Bug label Sep 28, 2014
@CoolOppo
Copy link

CoolOppo commented Oct 9, 2014

@dieulot Why haven't you commented on this yet? You added the "Bug" label 12 days ago, and it seems like you may not have realized that this is a pull request.

@dieulot
Copy link
Owner

dieulot commented Oct 13, 2014

I did notice that, but I haven’t had time to look into it. Don’t fret, I’m just a bit overwhelmed by all there is to do with InstantClick’s development at the moment.

@CoolOppo
Copy link

Alright, no problem. I was just commenting to make sure because it seemed to me like you may not have noticed. I didn't want that to come off in a negative manner.

@dogweather
Copy link

👍

@franciscop
Copy link

As a temporary fix to anyone suffering this: load all scripts on all pages (if different <script> tags are being used).

Reproduction:

<!-- page1.html -->
<script src="fna.js"></script>
<script>fna();</script>

<!-- page2.html -->
<!-- no fna.js -->
<!-- no fna() -->

Go to page2.html, then click a link that takes you to page1.html. The second script is faster since the first one has to load from a remote server. So the above configuration will throw the error ReferenceError: fna is not defined.

Now, to fix it, do this:

<!-- page1.html -->
<script src="fna.js"></script>
<script>fna();</script>

<!-- page2.html -->
<script src="fna.js"></script>
<!-- no fna() -->

@dieulot would you consider other maintainers? At least for bugfixing/accepting PR. This project is very cool but it's a pity that it has some unresolved bugs.

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.

6 participants