Change 701600 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/QuickSurveys@master] Make Vue rendered surveys the default

https://gerrit.wikimedia.org/r/701600

Change 702434 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[operations/mediawiki-config@master] Use Vue.js for QuickSurveys on available wikis

https://gerrit.wikimedia.org/r/702434

Change 702434 merged by jenkins-bot:

[operations/mediawiki-config@master] Use Vue.js for QuickSurveys on available wikis

https://gerrit.wikimedia.org/r/702434

Mentioned in SAL (#wikimedia-operations) [2021-07-01T18:43:03Z] <urbanecm@deploy1002> Synchronized wmf-config/InitialiseSettings.php: 7995f7abe3b94eb0326064cbbd1d3111f8f21365: Use Vue.js for QuickSurveys on available wikis (T285890) (duration: 01m 09s)

Dear performance team,
Could you advise on the best way to load code with the new Vue.js based QuickSurveys in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/QuickSurveys/+/701600?

My understanding is it's not okay to load WVUI on page load so it was done this way by design. What is the official performance team recommendation here?

QuickSurveys has a ext.quicksurveys.init module which checks if any surveys are active.
In this patch, the ext.quicksurveys.lib module is then loaded to determine whether a survey should be shown, if a survey can be shown, then ext.quicksurveys.lib.vue is pulled down to display the survey (when off the critical path).

I could merge ext.quicksurveys.lib.vue into ext.quicksurveys.lib but that would mean Vue is loaded on the critical path.

I could alternatively follow this patchset up with a modification to ext.quicksurveys.init to include a file with a single requestIdleCallback / mw.loader.using call to pull down the rest of the code.

We'd generally recommend measuring the paint time (e.g. from responseStart to survey visible) and cpu cost (page load with-vs-without a survey active) before/after the Vue change. I understand this shouldn't stay on OOUI, but it's still a good oppertunity to be able to document the experience and numbers to talk about publicly.

In T285890#7212447, @Jdlrobson wrote:

I could alternatively follow this patchset up with a modification to ext.quicksurveys.init to include a file with a single requestIdleCallback / mw.loader.using call to pull down the rest of the code.

All mw.loader generally does here is start an async network fetch. Using rIC would not meaningfully defer a cost there. This pattern emerged once in Popups, and I've described in T190950/T248718 that this should be removed. If something is loaded unconditionally on a given page, it should be queued server-side and loaded in the main batch. That batch is always async and graceful. It something can be lazy-loaded, then the fetch should happen from a user-initiated action. There isn't much need for a middle-ground there in my experience, but I'd be happy to see a case where it helps one of our key metrics.

Could you advise on the best way to load code with the new Vue.js based QuickSurveys? […]

Looking at the main branch, we currently have:

• QuickSurveys currently registers 4 modules (lib, lib-views, lib-vuejs, and init).
• The server-side enqueues ext.quicksurveys.init, only on page views with non-zero surveys available to "this page on this wiki".
• The ext.quicksurveys.init module is an empty shell with ext.quicksurveys.lib as sole dependency if there are non-zero surveys configured on "this wiki".
• The ext.quicksurveys.lib module has numerous dependencies and source files for 1) sampling and selecting active surveys, 2) kicking off the display part (loading spinner). If a survey is selected for the current user and page view, it will display the spinner and load the survey.
• Loading a survey means calling mw.loader.using( [ uiModule, surveyModule ] ) where uiModule is either ext.quicksurveys.lib.vue or ext.quicksurveys.lib.views, and surveyModule is code provided by another extension. After these are loaded, the UI displays the survey.

I see two redundancies here:

1. The ext.quicksurveys.init module seems obsolete here.

It certainly looks as if it's conditional nature is redundant with the per-page enqueue logic, but I remember T213459. The point is that the per-page logic ends up CDN-cached. So while the per-page logic should suffice on its own, we have the site-wide logic to make sure that when a survey is taken out, that cached pages views quickly end up not bringing in the lib payload.

But, that said, it is not a logical bundle by itself or fulfulls no contract that's different or independent from another module. I'd recommend merging ext.quicksurveys.init into ext.quicksurveys.lib and applying the conditional to its bundle definition instead. The indirection of the client-side is not needed here.

2. ext.quicksurveys.lib.vue and ext.quicksurveys.lib.views fulfill the same site-wide contract.

I understand this is "temporary", but as a general pattern I'd recommend avoiding this approach. Bundle endpoints should generally represent logical contracts between pieces of code (server and client, or two client-side features that are not always present at the same time), not specific internal details of what they currently contain. If something varies by user preference, then such implementation detail becomes part of the contract (like in Vector right now), but that is the exception, not the rule. By default things should generally be consistent site-wide and not expose alternate pathways like this.

In other words, I would have temporarily defined ext.quicksurveys.lib.views in a dynamic way and vary its definition based by wgQuickSurveysUseVue. This generally also ensures metrics and inter-dependencies are more stable and less likely to cause issues when caching or train rollbacks happen. I believe in this case it was all done safe and well, but it's a category of risk that you can eliminate which seems a plus (and saves on startup manifest, which doesn't matter so much in the micro here, but makes a difference holistically if other teams were to follow the same pattern day-to-day).

My understanding is it's not okay to load WVUI on page load so it was done this way by design. What is the official performance team recommendation here? […] I could merge ext.quicksurveys.lib.vue into ext.quicksurveys.lib but that would mean Vue is loaded on the critical path.

Indeed, that would be bad.

I think aside from the two redundancies laid out above, the general loading order and strategy laid out since T213459, and as currently implemented, still looks good. I'm not sure anything needs to change?

Perhaps I'm overlooking something in the patch, though. I did not spot any change in how or what is loaded (apart from switching out OOUI for WVUI/VUe). It seems the same requests still happen and in the same moments in time. Is that right?

I think aside from the two redundancies laid out above, the general loading order and strategy laid out since T213459, and as currently implemented, still looks good. I'm not sure anything needs to change?

T213459 only concerns when a survey is not active. However, when a survey is active, in production, loading a survey loads the entire OOUI library on page load regardless of whether a survey is shown This was long documented in T159738.

The new Vue.js implementation is a lot fewer bytes than the OOUI library, so regardless of the course of action we take here we've improved things. I am getting a lot of mixed messages from the performance team however, since we load Vega on pages with a graph (T272530) and we are being told not to load WVUI on startup yet WVUI is smaller than Vega.

With the mw.loader.using pattern I've introduced here, WVUI code is only loaded when the survey is shown, the only code loaded on startup is a small script which checks if the survey shown be shown a survey

So basically the choice is between
Load rendering library (WVUI/OOUI) if a survey is enabled and the user is eligible for the survey (current patch)
Load rendering library (WVUI/OOUI) if a survey is enabled (revised patch and status quo)

This pattern emerged once in Popups, and I've described in T190950/T248718 that this should be removed

A page on wikitech or mediawiki with established recommendations would be really useful to avoid this confusion about which patterns are recommended and which are not.

As a heads up this conversation will come up again shortly in T284463, as we plan to use Vue.js to build the sticky header for the new Vector experience, and will need to identify a suitable loading strategy.

I am getting a lot of mixed messages from the performance team however, since we load Vega on pages with a graph (T272530) and we are being told not to load WVUI on startup […]

The current state of the Graph/Vega extension goes against our performance guidance and, more importantly, it is in violation of numerous architecture principles. When the Graphoid service was removed (T211881), the resulting status is that:

• Significant increase in page weight (large HTML/JSON, embedded in the critical path even before the article content).
• Significant increase in page load time (unconditional async sequence to initialise Vega and render the graph etc.).
• Content has become inaccessible to Grade C page views (e.g. in modern browsers on slow connections, modern browsers with JS errors/blocked/disabled, and older always-GradeC browsers).
• Content has become unavailable to image search and search engines more generally.
• Content has become unavailable to all re-users of our core data outside the online canonical Wikipedia.org experience. This includes Siri, Apple Lookup/Dictionary, Kiwix, IPFS, Safari Reader mode, Tor browser, and most other third-party apps.

As I understand it, only the first two points were known to product leadership, and those were accepted at the time with the understanding that it would be "temporary" and affecting only a minority of pages (those with Vega-powered graphs). I believe teh reason for this decision was because Graphoid had become operationally unstable and it would have taken considerable effort to upgrade and get back up to current standards, and thus it was decided to invest that effort toward a longer-term solution instead. That investment is tracked at T249419, which once it comes to fruition moves graph rendering back to the server-side in a more sustainable way. As such, I expect our presently-ignored post-hoc perf review at T240626, to be superseded by T249419.

We have at no point supported the client-side rendering of graphs. I hope this clarifies that there are no mixed messages. If "a lot" covered anything else, let me know. See also our general perf guidance.

Change 701600 merged by jenkins-bot:

[mediawiki/extensions/QuickSurveys@master] Surveys are always rendered via Vue.js

https://gerrit.wikimedia.org/r/701600

Change 804014 had a related patch set uploaded (by Catrope; author: Catrope):

[operations/mediawiki-config@master] Remove unused setting wgQuickSurveysUseVue

https://gerrit.wikimedia.org/r/804014

Change 804014 merged by jenkins-bot:

[operations/mediawiki-config@master] Remove unused setting wgQuickSurveysUseVue

https://gerrit.wikimedia.org/r/804014

Mentioned in SAL (#wikimedia-operations) [2022-06-15T20:08:27Z] <catrope@deploy1002> Synchronized wmf-config/InitialiseSettings.php: Config: [[gerrit:804014|Remove unused setting wgQuickSurveysUseVue (T285890)]] (duration: 03m 38s)