-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Rework client-side language handling to improve availability #21587
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
Rework client-side language handling to improve availability #21587
Conversation
| // Fetch locale JSON without jQuery to allow fetching before jQuery is loaded | ||
| function loadLocales() { | ||
| const xhr = new XMLHttpRequest(); | ||
| xhr.open('GET', '" . \jsescape($locales_path) . "', false); // synchronous request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Synchronous ajax requests are deprecated and very bad for UX as the whole browser freeze during the request.
Synchronous XHR requests often cause hangs on the web, especially with poor network conditions or when the remote server is slow to respond. Synchronous XHR is now deprecated and should be avoided in favor of asynchronous requests.
I understand that there is probably some constraint that lead you to this solution but IMO we should look for an alternative here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Synchronous ajax requests are deprecated and very bad for UX as the whole browser freeze during the request.
I am well aware, but so does the initial page load which may have something we can claw back the same performance from. With this, we are talking about maybe 40ms on my side one time and then maybe 2ms after it is cached. Even when I am in development env and it doesn't get cached, I don't notice any more of a delay than I usually see.
I understand that there is probably some constraint that lead you to this solution but IMO we should look for an alternative here.
I have several alternatives. None of which people would be happy about for a bugfix version and I think we will be discussing the frontend design decisions soon. The things I am working on related to frontend/performance are mostly short-term solutions for 11.0.X in my mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionally, there is no difference between a synchronous request or awaiting a promise-based one. We need these locale strings to be immediately loaded before any of our JS that uses the translation functions gets loaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 seconds with async calls is insane, as is the main locales taking nearly a second. I will test again with my laptop a bit later this morning to get a little more realistic timings with my PR.
It sounds to me like we already have the one main "downside" of a SPA, being that we have some data that needs loaded client-side before any rendering can take place, without any of the benefits of SPA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Over wifi, the main locales are taking 100-150ms. If I throttle to 4g speeds, it takes 900ms. By the time the locales start loading, the main page is already rendered even if it may not be fully interactable yet.
| $script = " | ||
| // Fetch locale JSON without jQuery to allow fetching before jQuery is loaded | ||
| function loadLocales() { | ||
| const xhr = new XMLHttpRequest(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to use XMLHttpRequest instead of the fetch API here?
Fetch API has been the standard for some times, see: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API
The Fetch API provides an interface for fetching resources (including across the network). It is a more powerful and flexible replacement for XMLHttpRequest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fetch is promise-based only, which means async. It could have been done with some more work but I don't see the issue for something small like this which I would want to tear out and completely redo as soon as I am able.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A solution could be to have synchronous <script> tags that refers to a stateless PHP controller that serves a script that defines the locales (with a i18n.loadJSON() call), loaded right after the lib/base.js script. The browser would probably use parallel requests to load them, that would be better that synchronous requests executed sequentially.
Another solution could be, if it is possible, to execute async requests as we are doing currently, but use a proxy object in _*() functions to wait for the corresponding response when it is not yet received.
I'll take a look. I was trying to keep things as close as possible to how they are currently, keeping in mind that I struggle to see any kind of fix as more than a short-term solution. I considered doing this kind of blocking call from the translation functions but thought that loading it before it is needed would be a smoother experience. With a more long-term solution, I had already though about making the translations reactive so that you could easily switch languages without needing a page reload, but that really applies with a substantial frontend rework. Finally, I considered allowing |
ffee8f0 to
ac802aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even with many plugins, the latest solution seems OK to me. The initial load (without cache) may indeed be important, but this is mostly due to the fact that there are a bunch of JS files already in the load queue at this moment, and the browser does not request all resources at the same moment.

Checklist before requesting a review
Description
Fixes #21434