Closed joshkadis closed 6 years ago
No specific reason. Please contribute.
Note that current master seems to be in a broken state due to a refactoring (one big untested file to several small tested pieces). If you want to make a PR, please do that on the current master following the "in-progress" refactoring :)
Sounds good! Thanks. Will check back for the refactoring.
Reopening to remember to handle that.
@joshkadis @MoOx Is this fixed by 09f14fc?
No idea.
@lacrioque ?
I'm sorry I've just seen it a short while ago. The problem here is, that external scripts are loaded asynchronously. So we would need another event to give inline-scripts the possibility to react on that. This however needs promises or multiple callbacks. This library should be without any dependancies and thus i couldn't just add Promises.
@lacrioque It's been a while but I vaguely recall having solved this somehow. I will have some free time starting next week to remember what this is all about.
🚀 time travel back to 2015
Whilst looking into #117, I noticed that lib/eval-script.js
claims to force inline external scripts to execute asynchronously, but actually forces them to be synchronous: https://github.com/MoOx/pjax/blob/09f14fc86cd161f3a27e988b56212322b08e2a16/lib/eval-script.js#L21
Does this not mean that, assuming the script tags in the new document being switched are written in the correct order (i.e. inline scripts that depend on external script dependencies come after the external dependency script tag) as per @joshkadis' original example, this should be fixed? Inline script tags are parsed in the order in which they are found in the new document, so the source ordering should remain as intended.
I haven't tested this assertion yet, but will aim to try and do so.
I did a quick test, which appeared to be like you said. The comment on that line would be wrong, but the behavior seems to be correct.
If that's true, I think we should leave it as synchronous, and just change the comment. That behavior would reflect how a new page would be parsed, which is by default synchronous.
I have actually just changed that comment in #118, so maybe that's all we need to do to close this 😄
@joshkadis @lacrioque Is there anything we're missing here?
@joshkadis @lacrioque I'm going to assume for now that this was fixed by 09f14fc, and the comment was just backwards (and was fixed in #118).
If that's incorrect, please let us know and we can reopen this.
I am still having this problem if the new request fragment has something like this:
<script src='/javascript/external.js'></script>
<script>
externalDefinedVariable();
</script>
The script "/javascript/external.js" is still being loaded asynchronously. I will try to investigate it deeper, but some hints would be helpful :)
The
eval-script
module is slick! But it doesn't address if the inline JS to eval is dependent on an external script that's loaded in the new DOM elements, e.g.Would you be open to a pull request for that? Or is there a reason why you wouldn't want to do that?