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

Defer breaks autoload (when loading page without having resources cached) #2102

Closed
BrainStone opened this issue Oct 23, 2019 · 29 comments · Fixed by #2104
Closed

Defer breaks autoload (when loading page without having resources cached) #2102

BrainStone opened this issue Oct 23, 2019 · 29 comments · Fixed by #2104
Labels

Comments

@BrainStone
Copy link

Information:

  • Prism version: 1.17.1
  • Plugins: autoloader
  • Environment: Browser

Description
When the page is loaded without the JS resources being cached and the scripts are marked with defer then the syntax highlighting is not applied.

Example
Without defer (workking): https://jsfiddle.net/f5vj903e/
With defer (not working): https://jsfiddle.net/f5vj903e/1/

BrainStone added a commit to AuraDevelopmentTeam/AuraDevHomepage that referenced this issue Oct 23, 2019
@RunDevelopment
Copy link
Member

Cannot reproduce.
What browser are you using?

@BrainStone
Copy link
Author

Very weird. I now also can't reproduce in a minimal example.
Would it help if I linked you a live page where the issue appears?

Also I am using FireFox 70.0 (64-Bit)

@BrainStone
Copy link
Author

Well, in any case this is the page: https://dev.aura-dev.team/documentation/test
Reloading without the cache breaks the page in funny new ways every time.

@RunDevelopment
Copy link
Member

Can kinda reproduce.
I don't see any "funny new ways every time", I just see that the Autoloader isn't configured correctly. Normally, the Autoloader will know from where to load languages by using its own file path. I'll fix that in a minute.

However, could you please describe what breaks? In both FF 70 and Chrome 77, I only see that the PHP code isn't highlighted because prism-markup-templating.min.js failed to load. Everything else seems to work fine.

@RunDevelopment
Copy link
Member

Can we even fix this?
Right now, we use these lines to get the script element which contains the Autoloader. Because of defer, the Autoloader script might get executed after some additional scripts have been loaded meaning that it isn't the last script element in the collection anymore.

We could use document.currentScript but that isn't supported by IE11 and I don't want Autoloader behave differently depending on the browser.
Any ideas @mAAdhaTTah @Golmote ?

@BrainStone
Copy link
Author

Sometimes just realoading will also create funky results like this:

grafik
(I scrolled them down but you can clearly see an additional line at the end but the box being resized to one less line (as it should be))

Occationally I also get that the toolbar extensions don't load properly (like just one or none at all). I couldn't replicate it at work. Probably due to the internet being a lot faster here. I'll try to capture some more funkyness at home.

@RunDevelopment
Copy link
Member

This is probably because all the different Prism files have to be loaded in a specific order and the defer creates a race condition. Unfortunately, you can't specify defer-dependencies, so you have to either bundle them into one file or load them synchronously.

@BrainStone
Copy link
Author

defer gurantuees that the scripts are executed in order.
See https://www.growingwiththeweb.com/2014/02/async-vs-defer-attributes.html#script-defer for example

@RunDevelopment
Copy link
Member

defer guarantees that the scripts are executed in order.

I didn't know that. That's for pointing that out.

I can reproduce the issue you describe now albeit each with a low probability of occurrence. Everything seems to be working fine in Chrome and Edge, so maybe it's a FireFox issue?
Since the issues occur non-deterministically, I'm pretty sure that we have ourselves a race condition but I don't know where.

I'll look more closely at this soon. If you find any hints or patterns, please share.

@BrainStone
Copy link
Author

I'll be doing some additional testing myself.
If you need a page to test, let me know and I will make a more permanent page for this.

@RunDevelopment
Copy link
Member

That would be great. Thanks!

@mAAdhaTTah
Copy link
Member

mAAdhaTTah commented Oct 24, 2019

I am seeing this in the console:

prism-autoloader.min.js:1 GET https://unpkg.com/mermaid@8.4.0/dist/components/prism-markup-templating.min.js net::ERR_ABORTED 404

What is mermaid and why are you serving Prism from it?

@mAAdhaTTah
Copy link
Member

Ohhhh, I see, the autoloader is using the last script to get the path, and the path is wrong, because defer.

@BrainStone
Copy link
Author

BrainStone commented Oct 24, 2019

Couldn't it be fixed by iterating of over the list of scripts and checking if the URL is for the autoloader (or the main script) in the first place?

@mAAdhaTTah
Copy link
Member

I don't think that would work if Prism was bundled together, but what we could do (looking at the code) is query for an element with data-autoloader-path and use that value instead of assuming it's the last script in the list. Maybe we can look into querying other things to try and figure out the path...?

In the short term, if you put the set of Prism elements last (with the autoloader dead last) instead of mermaid, that should fix it. Alternatively, you could add data-autoloader-path to the mermaid script and the autoloader will use the value found there.

@BrainStone
Copy link
Author

I think I tried that and it didn't quite work.
Though it might be that it just were the other race conditions that were acting up and not that particular issue.

But how about you try these things in order:

Search all scripts for:

  • data-autoloader-path
  • Prism Autoloader URL
  • Prism URL

Use script.this as a last resort and then just fail with a nice error message advising to add data-autoloader-path.

And be sure to properly document that.

@RunDevelopment do you want me to open a separate issue for the other race conditions?

@RunDevelopment
Copy link
Member

Nah, just leave it here. They are both caused by defer.

Also, regarding getting the current script: We have the same issue in Prism Core and every plugin which can be configured using script attributes.

The only reason we don't use currentScript is IE11 (I assume), but there we could use this error-throwing idea of this polyfill. Wrap it all into something like Prism.util.currentScript() and done. This won't work if people inline the Prism source code on their website, but who's gonna do that?

We could also just give IE11 the short end of the stick #1578.

@BrainStone
Copy link
Author

BrainStone commented Oct 24, 2019

I mean having a util method is a great idea.

While I do understand not wanting to use currentScript I think having it as an additional fallback would be a good idea.

Btw Opera Mini doesn't support it either: https://caniuse.com/#feat=document-currentscript

P.S.: Since this whole issue only really appears when using defer I'd suggest using the method of Prism Core and just documenting that if you wish to support IE 11 or Opera Mini, you can't use defer or async.
I think that sounds like a fair approach.

@BrainStone
Copy link
Author

Now that I'm back home I noticed that the issues are worse. The difference between work and home is that my home internet is a lot slower.
So my guess in regards to the race condition would be that the script loads (an executes) after the onPageLoad event (or whatever other page load event you are using). And since the listener got registered after that event it never triggers.

@BrainStone
Copy link
Author

BrainStone commented Oct 24, 2019

So I created a standalone HTML page that displays the bug (not minimal, but I think other scripts help bringing the issues out).
testpage.zip

It helps if you put the page on a (slow) webserver. As locally I can't seem to reproduce any of the issues except the wrong script tag issue, but online they still all happen.

BrainStone added a commit to AuraDevelopmentTeam/AuraDevHomepage that referenced this issue Oct 24, 2019
Requires:
- prism to be loaded last
- prism autoload to be the last script

Knowledge gained from PrismJS/prism#2102
@RunDevelopment
Copy link
Member

Alright, I think I know what causes the race condition.

Since the readyState of the document is interactive, Prism Core uses requestAnimationFrame to highlight all code blocks.
The problem with this is that this means that all plugins have to loaded and executed before the next frame. This is the reason why some plugins are sometimes active and sometimes not.

(You can verify that this is indeed what's going on by setting a breakpoint in e.g prism-linenumbers.min.js and you'll see that the breakpoint sometimes hits after all code blocks have been highlighted.)

This is a bit of an issue because we'll have to wait for resources that might be loaded.
Unfortunately, defer doesn't work on inline scripts:

<script defer data-manual src="https://cdn.jsdelivr.net/npm/prismjs@1.17.1/prism.min.js" integrity="sha384-ccmyu9Bl8HZLIVEUqF+ZzcZTBPB8VgMI2lQpOsNDOvro/1SfRnz3qkub0eUxof1s" crossorigin="anonymous"></script>
<script defer src="https://cdn.jsdelivr.net/npm/prismjs@1.17.1/plugins/autoloader/prism-autoloader.min.js" integrity="sha384-xF5Qt8AUh+k8ZzozF9d1iDRKeeP1m9PPJKKhy3R/O4+5JccihNLvy1fIuGnkye7+" crossorigin="anonymous"></script>
<script defer src="https://cdn.jsdelivr.net/npm/prismjs@1.17.1/plugins/line-numbers/prism-line-numbers.min.js" integrity="sha384-g0u6zLvZF3g2I+/vETu7YYk74PXoDBNGy5qtL04/uW6PJGMDeax43A5hFa55xaAT" crossorigin="anonymous"></script>
<script defer src="https://cdn.jsdelivr.net/npm/prismjs@1.17.1/plugins/toolbar/prism-toolbar.min.js" integrity="sha384-Q7kIVs9c3Zyij/OpKzuVFFpRTTXHL3qytFywO0AtJdmisfKKSEsnoc7pHfkxgXZX" crossorigin="anonymous"></script>
<script defer src="https://cdn.jsdelivr.net/npm/prismjs@1.17.1/plugins/copy-to-clipboard/prism-copy-to-clipboard.min.js" integrity="sha384-V+ZXzru/5DpvSuLtIS2SctoAyvpigW+zF56aiGLPc+PvQxO3EkpeYDUw6t1Y6d9y" crossorigin="anonymous"></script>
<script defer src="https://cdn.jsdelivr.net/npm/prismjs@1.17.1/plugins/show-language/prism-show-language.min.js" integrity="sha384-xnJRKz3VKJloaoR0FNJVmbJ55eTSeuztu0Okhd9vvz2hblDQc0UJWVkj3sIikslo" crossorigin="anonymous"></script>
<script defer>Prism.highlightAll();</script>
<!-- defer won't do anything -->

So, instead of using the requestAnimationFrame path, we should use the DOMContentLoaded path in Prism Core because deferred scripts are guaranteed to be executed before that event.
And to figure out whether the current script is deferred or not, we need currentScript. (yay, full circle)
I'm imagining something along the lines of the following as a fix.

	if (document.readyState !== 'loading' && !script.defer) {
		// requestAnimationFrame
	} else {
		document.addEventListener('DOMContentLoaded', highlightAutomaticallyCallback);
	}

Have I verified that this fix works?
No.
Why not?
Funny story. As it turns out: the moment you add an inline script everything works. No race condition, it just works. Srsly, try it. Just add <script>/* how? */</script> anywhere in the test page. (btw <script></script> doesn't work. Seems like someone at Mozilla wanted to optimize)
How's that as a fix? Just add <script>/**/</script>.

@BrainStone
Copy link
Author

BrainStone commented Oct 24, 2019

Reordering the scripts so that autoload is actually the very last script seems to work too btw.

But glad you found the issue.

I'd go with this snipped (var script = document.currentScript || [].slice.call(document.getElementsByTagName('script')).pop();) to detect the script and add the compatibility issue with IE11 (and Opera Mini) + defer

Great job for figuring this out.

Also document that async is deadly (executing scripts out of order).

P.S.: I'm willing to test the fix. The page is not live anyways.

@mAAdhaTTah
Copy link
Member

Browsers are weird

@RunDevelopment
Copy link
Member

Ok, I just verified that the fix works. I'll make a PR.

RunDevelopment added a commit that referenced this issue Oct 25, 2019
This fixes a race condition that can occur when deferring Prism components.

For more details see #2102.
@BrainStone
Copy link
Author

Do you have an ETA for 1.17.2?

@RunDevelopment
Copy link
Member

This is gonna be part of Prism 1.18.0 which will be released soon. Maybe a week or so?

@BrainStone
Copy link
Author

Ah perfect. I'll just wait for that.

@BrainStone
Copy link
Author

How's 1.18 coming along?

@RunDevelopment
Copy link
Member

Sorry, I've been quite busy and hadn't much time.

We're currently wrapping up the last PRs which are going to be included.

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 a pull request may close this issue.

3 participants