Page MenuHomePhabricator

Lightweight audit of Vue code and Codex implementation in GrowthExperiments
Closed, ResolvedPublic3 Estimated Story Points

Description

The Growth team has been working on migrating existing software (mentee overview module on Special:MentorDashboard, T300532) and creating new projects (Impact module on Special:Homepage, T222310).

While working on those projects, we've periodically reached out to Design-System-Team colleagues and received useful feedback and code review comments (thank you!). That has mostly happened in the context of fairly narrow questions. This task is to request a high-level review of our Vue code, how we've used Codex components. The reason for doing that is that there aren't many examples to draw upon in the MediaWiki ecosystem, so it's hard to know if we're doing things the best way.

Some goals for this review:

  • ensure we're following best practices and latest guidance, per the product development best practices playbook
  • see if there are areas to improve our code
  • identify components that could be upstreamed from GrowthExperiments code
  • document pain points in working with Codex / Vue in MediaWiki

I could see this audit working in a few ways:

  • a synchronous meeting, 1:1 with a DST engineer and one or more Growth engineers to walk through code and talk through what we built
  • write up questions/notes asynchronously to allow more time from DST engineers to look through the GrowthExperiments codebase
  • a combination of the above

I'm optimistically hoping this would be a ~2 hour time commitment from both sides.

If this doesn't fit into your schedules this quarter, that's not a problem, but if it does, let us know how you'd like to move forward with something like this.

GrowthExperiments Vue modules

Module name / script pathLink to codeNotes
ext.growthExperiments.Homepage.NewImpactBrowse codeUsed in Special:Homepage
ext.growthExperiments.MentorDashboard/MentorDashboardBrowse codeUsed in Special:MentorDashboard
vue-componentsBrowse codeBucket for Vue re-usable components and utils

Review questions

QuestionContextPossible actions
How to prevent an undefined property inside an SFC template that can go to production?The patch User impact: add disabled and unactivated state introduced an undefined variable when creating a new component: NoEditsDisplay.vue. Inside the template tag there's a property bind using "mode" variable: :size="mode !== 'desktop' && 'sm'" which is undefined.The error could had been caught with a unit test but a a linting error should be also possible and avoid bugs in non-tested components
What's the recommended practice for writing and loading styles for both server and client?Growth Team built a skeleton interface for the user impact module to improve the javascript and data loading experience (T321675: NewImpact: Create skeleton loading state). Styles from the client components were reused and some ad-hoc styles were added for the markup rendered on the server (see patch User impact: add skeleton markup while JavaScript loads). The goal is to build interfaces that present some minimal content while javascript loads.Design Systems Team is working on CSS-only versions of Codex components that can be loaded in advance of JS initialization, something we hope to have ready soon. For components developed by Growth, it's not ideal but you could move LESS styles into separate files and then ensure that the styles get loaded on the server. This is the approach that Structured Data followed when developing MediaSearch. Finally, this seems like a possible new feature to consider adding to ResourceLoader: RL is already pulling out the styles from Vue components, so maybe we can enable loading them independently from JS. In general it would be great to allow addModuleStyles to work with any RL module, whether it also has a JS portion or not.
What would be the best setup for reusing interfaces between desktop and mobile?GrowthExperiments Special:Homepage contains several JS modules (some using OOUI some Codex) that can be enabled based on different configurations. The modules have a consistent appearance across mobile and desktop sites. On mobile the content is rendered inside an overlay provided by MobileFrontend. Advice on how to reuse interfaces across modules in the same Special page and across different special pages is appreciated. The current setup creates a new Vue mwApp for each platform. The components are then tweaked based on an injected property. Working on a functional setup of vue-router with some mobile navigation system could be helpful. Also for having several "views" on the same page and avoid the mount boilerplating.I would recommend trying to solve this problem in CSS as much as possible. Say you need slightly different styling on a component based on the mode property of the top-level app (overlay vs regular mode, etc). Instead of passing the mode throughout layers of components or using provide/inject, would it be possible to just write your component CSS so that a different set of styles come into play when the outer App or Layout component (which is aware of the "mode") adds a different CSS class? For example, inside your Scorecard component, you could write some nested LESS rules that only apply when the parent element has an .mode--overlay modifier class.
How to consistently display OOUI and Codex icons and fonts?GrowthExperiments features mix OOUI and Codex interfaces. Having a consistent way to display icons and fonts will help to create consistency while the two libraries coexistI think the answer here is to increase reliance everywhere on the Codex design tokens. Some work is currently underway to make it easier to use them inside MW skins and extensions (see T325237).
How to allow portability of GrowthExperiments Vue components?Allowing portability of GrowthExperiments components to other contexts like Codex or an automated documentation tool could be helpful. Is there a Vue "recipe" that GrowthExperiments could adopt and would facilitate: (a) upstream of components to Codex (b) Autogenerated documentation with a vite or similarGrowth could consider developing its Vue components in an external library (outside of MW) similar to how Codex is developed. This would allow a couple of benefits: components could be written in a standard way, you could use Typescript and other front-end build tools like Vite; you could compile your components for use inside of MW but also deploy a stand-alone documentation site using something like Vitepress, and you could split out your CSS styles into a separate file that you could load independently. If Growth has a large-enough amount of custom components, this approach might be worth exploring. In this situation a lot of the existing Codex Vite configuration could be re-used, and DST could help you with initial setup.
How to monitor application errors, performance, memory leaks, etc?Are there recommended patterns (eg: patch Instrumentation: Monitor navigation duration, transferSize, first paint), tools for CI or local development (eg: Vue devtools) to monitor Vue specific app metrics.Consider using the logger plugin that you already have and enhancing it with a few additional methods – that will also help to de-clutter your components.
Is there a way to programmatically spot uneeded reactivity?Using Vue's reactivity API is meant to be used when the nature of the data is changing. However in the GrowthExperiments extension some features load a significant amount of data from the server in JS config vars and then mount a Vue application with the data in place so reactivity is not really useful. Is there a way to programmatically find this situation? Is it relevant from a performance point of view?Vue has some tools for this (v-once, etc) but I'm not sure you'd see any performance benefit unless you are rendering a lot of components in the user's browser.

Event Timeline

egardner moved this task from Inbox to Needs Refinement on the Design-System-Team board.

SFC vs JS/TS components: Refactoring from a Vue 3 composition API + setup recipie to use the Vue render API is tedious.

I'm not clear on what this means. By "the Vue render API" do you mean the options API? Most older Vue code in MediaWiki uses the options API, and most newer Vue code (including Codex's internal code) uses the composition API. But there's no reason you can't use the composition API in MediaWiki. Code like this works:

const { ref, computed } = require( 'vue' );

module.exports = {
    props: {
        label: {
            type: String,
            default: ''
    },
    setup( props ) {
        const hasLabel = computed( () => props.label !== '' );

        return {
            hasLabel
        };
    }
};

Undefined variables leak: an undefined property inside an SFC template can go to production

Could you explain this one in more detail?

SFC vs JS/TS components: writing SFC has the benefit of a "single place" for everything, however it comes with the drawback of not loading styles on the server.

This is an issue we're aware of, and we want to make it easier to load (part of) a Vue component's styles as render-blocking styles, but we haven't worked on this yet. This will also be important for server-side rendering. I'm not sure if there's a Phabricator task for this yet, but if not we should file one.

Lack of mobile tooling or documentation: It should be easy to use vue-router with MW mobile overlays.

This is also an issue that we (and the Web team) are aware of but don't have a task for yet.

Data flow and reactivity shave: In Vue "everything" is reactive, but in many of the GE code data is precomputed on the server.

Can you talk more about this? Why does this mismatch cause problems? Are you saying it's tedious to have to account for reactivity and potential for change in data that you know won't change?

SPA vs micro-frontends: To reuse interfaces between different special pages we need to create a new Vue mwApp and mount it.

Can you elaborate? If you're reusing components between special pages, then you should be able to use that component in two different apps. If your top-level component is the same, wouldn't you be able to reuse the code that creates the app object as well?

SFC vs JS/TS components: Refactoring from a Vue 3 composition API + setup recipie to use the Vue render API is tedious.

I'm not clear on what this means. By "the Vue render API" do you mean the options API? Most older Vue code in MediaWiki uses the options API, and most newer Vue code (including Codex's internal code) uses the composition API. But there's no reason you can't use the composition API in MediaWiki. Code like this works:

I was referring to the usage of the Vue.render function. But I'm realizing it seems it can be used with Composition API so there's no need for any refactor for using it, I presume it was a confusion reading some chunk of Vue code written with options API.

I elaborated the rest of questions and added context in the task description. Thanks!

egardner updated the task description. (Show Details)

I was referring to the usage of the Vue.render function. But I'm realizing it seems it can be used with Composition API so there's no need for any refactor for using it, I presume it was a confusion reading some chunk of Vue code written with options API.

This is the sort of thing that might be easier to discuss in the course of the actual component audit. In my experience manually writing a render function is not something you typically need to do, but I don't know the full context.

In general it is fine for Options API, composition API, and render functions to all co-exist in a component. We prefer the composition API in Codex because it works better with Typescript, but we tend to recommend that simple components inside MW use the Options API since it's more straightforward. If you wanted to gradually migrate a component over you could just add a setup() method to a component using Options API, just know that setup gets called first – before the lifecycle hooks, etc.

Moving this onto the DST sprint board in advance of our initial discussion to kick this off this week. I can post some initial findings here after we've had time to go through the code.

Change 885843 had a related patch set uploaded (by Sergio Gimeno; author: Sergio Gimeno):

[mediawiki/extensions/GrowthExperiments@master] WIP Vue audit

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

Per today's initial kick-off chat, DST is going to work on this for the next 1-2 weeks, with the goal of being finished before the end of February so that Growth has some time to process any recommendations before their new front-end focused intern comes on-board at the end of the month.

Myself and @Catrope will put together a document with some high-level findings/suggestions, and we will use the linked dummy patch in Gerrit in case we need to make any line-by-line comments or changes (the assumption is that this patch will never be merged).

egardner changed the task status from Open to In Progress.Feb 1 2023, 5:58 PM

I've added my initial review, which focused on the GrowthExperiments NewImpact module – though a lot of my suggestions are general. Gerrit patch here: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GrowthExperiments/+/885843

Gerrit is admittedly not the best venue for open-ended discussions, so I'm happy to have a follow-up meeting (next week maybe?) or start a thread in Slack to discuss any of the suggestions I made in code review. We can also continue the discussion here in Phab if that's helpful.

For visibility, here's my high-level recommendations for this code:

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GrowthExperiments/+/885843

I focused on the NewImpact module in my review, but a lot of this should apply generally. My main suggestions are about managing the data that flows though your app. Here are two things to consider:

  • Is it possible to cut down on the use of provide/inject and limit the amount of knowledge that individual components need to have of the big picture?
  • Relatedly, is it possible to move the API request logic into either App.vue or NewImpact.vue?

Right now, the API request is made in init.js and then the data is made available inside of the app using provide/inject. I see this as a workaround for our lack of SSR – you want to use the skeleton UI pending state until JS has initialized and all necessary data has been loaded.

However even with our current limitations around SSR, I think that your UI is conflating the no-js fallback UI element with the pending state UI. The former shows before JS initializes and for users who have JS disabled. The latter should only show to clients with JS enabled, after an API request has been made but before it has resolved. Even the design of the skeleton layout (which includes CSS animations) kind of implies that "something is about to happen" – seems less appropriate for users with no JS support at all.

What if you moved both the skeleton layout UI and the API request code into the main Vue app (allowing for cleaner code organization), and provided a simpler JS-free placeholder element in that region of the page before the Vue app is mounted? This placeholder might not need to look 100% identical to the skeleton; mainly I think you'd want to avoid jarring layout shifts. With the right design and styling, I believe that you could show the user a very natural and smooth-seeming transition between 3 states:

  1. Initial page load: show empty state (pre-JS initialization) defined in PHP & CSS; holds the space to prevent layout shift
  1. Vue app mounts and displays the Skeleton UI (loading indicator). This could fade in using some kind of transition so that it's not jarring to the user. In most cases this would happen a few ms after initial page load. The NewImpact component (which is the real place that needs the data as far as I can tell) could use an async setup() method to fire off the API requests and its parent could wrap it in a <suspense> component, showing the skeleton UI in the fallback slot.
  1. API request resolves (successfully or with an error) and the real UI displays. Since the NewImpact component won't even get rendered until the API request resolves successfully, you could write a more straightforward component without having to constantly check (in both template and logic) for the presence of the required data. All child components of NewImpact would benefit as well; if they receive the necessary bit of API data from NewImpact as props, they will never be in a state where it could be missing.

I mention Suspense since it sounds like you are considering it, but if you don't use it for whatever reason the above still applies, you'd just manage the pending and error states a little bit differently.

egardner set the point value for this task to 3.Feb 7 2023, 9:38 PM

Change 887977 had a related patch set uploaded (by Sergio Gimeno; author: Sergio Gimeno):

[mediawiki/extensions/GrowthExperiments@master] Vue components: add missing icon label

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

Change 887978 had a related patch set uploaded (by Sergio Gimeno; author: Sergio Gimeno):

[mediawiki/extensions/GrowthExperiments@master] User impact: add missing icon label for close button in scorecards

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

Change 887979 had a related patch set uploaded (by Sergio Gimeno; author: Sergio Gimeno):

[mediawiki/extensions/GrowthExperiments@master] Mentee overview: add missing icon label for close button in the legend tooltip

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

I've added my initial review, which focused on the GrowthExperiments NewImpact module – though a lot of my suggestions are general. Gerrit patch here: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GrowthExperiments/+/885843

Gerrit is admittedly not the best venue for open-ended discussions, so I'm happy to have a follow-up meeting (next week maybe?) or start a thread in Slack to discuss any of the suggestions I made in code review. We can also continue the discussion here in Phab if that's helpful.

Thanks a lot for the detailed review. I agree gerrit is not the greatest tool for this. I've replied within comments there to keep the context.

In regards to your concerns about application design and data flow; I think it's better if we can continue the conversation in a sync channel and then post some conclusions here. I would try to classify our findings in to some categories which include an action to be taken after. eg:

  • Defect: needs immediate fixing, it violates an existing best practice
  • Future defect: needs immediate fixing, it violates a missing best practice that we want to enforce, we should write the rule / practice somewhere
  • Broken window: does not need immediate fixing, it goes against some higher level programming principle, we should make sure the principle is mentioned somewhere or there's awareness about it
  • Code smell: does not need immediate fixing, it does not break any principle per-se but there's a suspicion it could cause problems
  • Scaling issue: maybe does not need immediate fixing, if we keep on doing this the application/codebase won't scale well

Change 888064 had a related patch set uploaded (by Sergio Gimeno; author: Sergio Gimeno):

[mediawiki/extensions/GrowthExperiments@master] NewImpact: remove undefined values in template

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

Change 888209 had a related patch set uploaded (by Sergio Gimeno; author: Sergio Gimeno):

[mediawiki/extensions/GrowthExperiments@master] NewImpact: reuse logger plugin for mw.track calls

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

Change 888064 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] NewImpact: remove undefined values in template

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

@Sgs and I had a conversation today to follow up on some of the points raised during the review of Growth Team code.

Here are a couple of take-aways:

  • Growth will look at re-organizing some of their client code that needs JS config vars from the server. Using Vuex or Pinia might be worthwhile here, especially in some future scenarios where multiple Vue apps on a single page might need to access the same store. One approach that could benefit code organization here is to initialize the Vuex store state with values that come from JS Config Vars in PHP, but then from that point forward all the Vue components only pay attention to the values in the store (and don't need to make calls to mw.config etc).
  • Vue Router may also be needed for future Growth projects – I believe that this library is in a similar situation where it has been approved but no one has needed it thus far, thus it has not yet been added to MW Core foreign resources.
  • Issues with undefined variables in SFC templates on-wiki. We may need to update our eslint-config-wikimedia rules to catch this; Codex does not have this problem because the Typescript compiler catches this issue for us.
  • Growth Team will explore using Suspense API with promises (rather than async/await, which is not compatible with our browser support matrix right now) and will report back on how this goes
  • We may have another opportunity for collaborative component development around the Tooltip component (listed here but not currently being worked on); this also relates somewhat to the Floating Button work (T278134: FloatingButton: Add FloatingButton component to Codex).
  • Growth has been maintaining its own set of Vue guidelines at: https://www.mediawiki.org/wiki/Extension:GrowthExperiments/Technical_documentation/Coding_conventions#Vue.js – we should probably consolidate these with the mian https://www.mediawiki.org/wiki/Vue.js/Guidelines page on MediaWiki.
  • Growth team will continue working on their own Vitepress component documentation tool, inspired by how Codex is documented. @AnneT is a good person to consult for ideas on improving this system.

Going forward, Growth engineers should feel free to CC DST engineers on particularly interesting or tricky patchs if they want some additional input.

Change 892954 had a related patch set uploaded (by Sergio Gimeno; author: Sergio Gimeno):

[mediawiki/extensions/GrowthExperiments@master] Vue tests: make icons truthy so they are rendered

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

Change 892954 abandoned by Sergio Gimeno:

[mediawiki/extensions/GrowthExperiments@master] Vue tests: make icons truthy so they are rendered

Reason:

Squashed in I88cbbb7228c373ad0873298949ce25318e18a3dc

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

Change 887977 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] Vue components: test CPopper

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

Change 887978 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] User impact: add missing icon label for close button in ScoreCard component

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

Change 887979 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] Mentee overview: add missing icon label for close button in the legend tooltip

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