DEV: Refactor stylesheet live-reloading (PR #13755)

We have had reports of tabs freezing in Firefox and reporting an error on this line. I haven’t been able to reproduce the browser error, but I suspect the forEach loop is at the heart of the issue, so I have replaced it with a (hopefully) safer call.

GitHub

This is an interesting theory: perhaps we are getting issues because many extra stylesheets are somehow inserted into the DOM and this loop goes through them all. In that case we should try and figure out how that’s happening.

I do have one suggestion though - rather than querySelectorAll you can use querySelector to find the first element, or null if it doesn’t exist. I think it will mean less work for the browser.

It’s exactly that, many extra stylesheets are getting inserted into the DOM in some conditions and they are also causing the forEach loop to go haywire. So the fix in this PR is only partial, we have to track down what is inserting the many stylesheets in the DOM. (I am still investigating, working on a separate PR.)

I kept querySelectorAll here, because with querySelector we would be replacing the first stylesheet only. When testing with many quick subsequent updates to a theme that was causing problems. It’s working better for me when the selector targets the last match, clones it with the new URL and removes it after a shorter timeout.

we could querySelector and change it to search for :last or something?

hmmm … how is it possible to have the same link more than once on a page? Can we repro that?

querySelector and change it to search for :last or something?

I’m not quite sure how much browser support there is for a [data-*]:last or equivalent selector.

Yes, I can repro links multiplying on the page, that’s what the change in DEV: Refactor stylesheet live-reloading by pmusaraj · Pull Request #13755 · discourse/discourse · GitHub, under refreshCSS is supposed to address.

With a combination of many stylesheets updating at the same time plus multiple quick changes, the 2s timeout in refreshCSS can sometimes not clean up old <link> tags. On the next file change, the forEach loop would find the duplicates, try to replace all of them, and so on, exponentially.

oh then maybe the big bug to fix here is to address the multiplication? if we ever see more than one link clean the old one up, log so we can fix the root cause?

@pmusaraj I am merging this as is cause I want to have the critical issue fixed (or at least the pain reduced) but lets follow up to find and mitigate the root cause.