Page MenuHomePhabricator

Test Vue 3 migration build with extensions/skins using Vue
Closed, ResolvedPublic

Description

Once the patch that upgrades MediaWiki's copy of Vue from 2.6 to the migration build of Vue 3.2 is ready, test that patch with each of the extensions below. Migration warnings can be ignored at this stage, but errors and other breakage should be fixed before the Vue 3 patch can be merged.

Extension/skin checklist:

Test plan:

  1. Change your initialization code to use Vue.createMwApp() instead of new Vue() (see T289020 for examples)
  2. Test that the Vue code in the extension still initializes correctly, and is attached to the DOM in the correct place
  3. Download this MW core change (git review -d 666434 in MW core)
  4. Test the Vue code in the extension, verifying that nothing breaks and there are no errors in the console (you may see warnings about deprecated things from Vue 2; that's fine, ignore those for now). Don't just load each Vue feature, but interact with it and test its functionality; some errors may only become apparent when a component is interacted with.
  5. Undo the changes you made in step 1 (e.g. by stashing your changes with git stash)
  6. Repeat step 2; if your extension uses Vuex or custom plugins, test those too
  7. (Optional) upload the changes you made in step 1 to Gerrit

Additional steps for extensions that use Vuex:

  1. Reapply the changes you made in step 1 (which you undid in step 5)
  2. Download this MW core change (git review -d 709125 in MW core)
  3. Test that features in the extension that use Vuex still work
  4. Change your initialization and store creation code to use the new Vuex 4 calling style described in T289103
  5. Repeat step 10

Related Objects

StatusSubtypeAssignedTask
Duplicate STH
In ProgressNone
ResolvedNone
Resolvedegardner
ResolvedLucas_Werkmeister_WMDE
Resolved Michael
ResolvedLucas_Werkmeister_WMDE
ResolvedLucas_Werkmeister_WMDE
OpenNone
ResolvedLucas_Werkmeister_WMDE
OpenNone
ResolvedCatrope
ResolvedCatrope
ResolvedSimoneThisDot
ResolvedCatrope
ResolvedCatrope
ResolvedCatrope
ResolvedJdforrester-WMF
DeclinedNone
ResolvedCatrope

Event Timeline

Catrope renamed this task from Test Vue 3 migration build with extensions using Vue to Test Vue 3 migration build with extensions/skins using Vue.Aug 17 2021, 12:17 AM
Catrope updated the task description. (Show Details)

Change 713605 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/extensions/GrowthExperiments@master] Homepage: Remove Start module and references to it

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

I've run into an issue with MediaSearch: when the SearchFilters component mounts, a method runs to set filter values according to what's in the Vuex store. The ref for each filter is used:

this.searchFilters.forEach( function ( filter ) {
    var currentValue = this.filterValues[ this.mediaType ][ filter.type ],
        ref = this.$refs[ filter.type ];

    // There there's code that uses that ref's properties/methods, like:
    ref.select( currentValue );
}

Instead of being an HTML element, the ref is a proxy object with a target and a handler. As you might expect, the target contains the original object (the HTML element), but I'm not sure how to properly access that. But these shouldn't be basic proxies, they should be a special type created by Vue called RefImpl that has a value property.

Plus, I didn't actually expect the refs to be proxies unless we're using the composition API - the docs don't seem to indicate any change in Vue 3 unless you're using the composition API.

(By the way, these filter refs are created in a v-for loop, and I already edited the code that sets that ref var to account for the fact that the refs are no longer placed in an array when they're created inside v-for.)

Another issue: the Tabs component has a default slot for Tab components, which used to come in the form of an array of VNodes inside the default slot, each representing a Tab. Now, there's a single VNode in that slot, and the tabs are only present as an array of child elements (not component instances), so this code inside the Tabs component no longer works:

initializeTabs: function () {
    // This was updated from this.$slots.default to this.$slots.default(), see
    // https://v3.vuejs.org/guide/migration/slots-unification.html#_3-x-syntax
    var tabs = this.$slots.default();
    this.tabs = {};

    // this.$slots.default() returns an array with a single VNode whose children are the tab
    // elements, so obviously this forEach doesn't work, but this code also needs the
    // component instance, which is null.
    tabs.forEach(
        function ( tab ) {
            this.tabs[ tab.componentInstance.name ] = tab.componentInstance;
        }.bind( this )
    );

    // ...more stuff
},

The docs don't indicate any change other than accessing a slot via a function as noted above.

Change 714102 had a related patch set uploaded (by Anne Tomasevich; author: Anne Tomasevich):

[mediawiki/extensions/MediaSearch@master] [WIP] Update code to Vue 3

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

egardner subscribed.

I've filed T290092 to remove the blocker in MediaSearch. We don't actually need the transition group component any more.

Thanks @egardner—that'll allow us to remove this blocker from MediaSearch. Still, I'd like to understand why the errors are occurring. Just to document what's going on:

The MediaSearch App component contains a <transition-group> element on each tab that contains 2 children, a custom component and a div that contains another custom component, both of which have unique keys:

<transition-group
    name="sdms-concept-chips-transition"
    class="sdms-concept-chips-transition"
    tag="div"
>
    <concept-chips
        v-if="enableConceptChips && tab === 'image' && relatedConcepts.length > 0"
        :key="'concept-chips-' + tab"
        :media-type="tab"
        :concepts="relatedConcepts"
        @concept-select="onUpdateTerm"
    >
    </concept-chips>

    <div :key="'tab-content-' + tab">
        <!-- Display the available results for each tab -->
        <search-results
            :ref="tab"
            :media-type="tab"
            @load-more="resetCountAndLoadMore( tab )">
        </search-results>
    </div>
</transition-group>

First of all, I'm getting a warning from the migration build that the children need to be keyed, even though they are:

[Vue warn]: <TransitionGroup> children must be keyed. 
  at <TransitionGroup name="sdms-concept-chips-transition" class="sdms-concept-chips-transition" tag="div" > 
  at <BaseTransition appear=false persisted=false mode=undefined  ... > 
  at <Transition name="sd-tab-fade-in" > 
  at <SdTab key="video" name="video" title="Video" > 
  at <SdTabs active="image" onTabChange=fn<bound onTabChange> > 
  at <MediaSearch>

Second, two errors are also thrown (for each tab), which seem to suggest that that sizing and classes are trying to be accessed or set for elements that don't exist yet. I'm not sure why the errors are occurring. Note that removing the <transition-group> element but keeping its contents resolves these errors. They are:

TypeError: child.el.getBoundingClientRect is not a function
TypeError: clone.classList is undefined

Change 716034 had a related patch set uploaded (by Anne Tomasevich; author: Anne Tomasevich):

[mediawiki/extensions/MachineVision@master] Update Vue init code to use Vue.createMwApp()

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

Change 716035 had a related patch set uploaded (by Anne Tomasevich; author: Anne Tomasevich):

[mediawiki/extensions/MediaSearch@master] Update code to unbreak use with Vue 3 migration build

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

MachineVision mostly works with the compatibility build as far as I've tested. The Tabs component in this app uses this.$children to grab tab data, which is deprecated but still functional, as opposed to the way we're doing it in MediaSearch.

But the <transition-group> element used in the Suggestion component is throwing the same TypeError: child.el.getBoundingClientRect is not a function error as I was seeing with MediaSearch. We can't really afford to get rid of the <transition-group> used in MachineVision from a UX standpoint.

A common theme I noticed between the broken <transition-group> usages in MediaSearch and MachineVision is that they're both the root element of a component, and they're both (indirectly) nested inside a <transition>. I haven't had time to dig deeper, but I wonder if that's part of what triggers this issue.

There is a breaking change in Vue 3 related to <transition-group>, but it doesn't seem like it would affect this, since you have set tag="div".

I installed MediaSearch, downloaded an older version of Anne's patch (patchset 2, the last one before she rebased it to remove the <transition-group>), and tried to run it in Vue 3. I was able to reproduce the TypeError: child.el.getBoundingClientRect is not a function error, and I found that it's happening because the underlying code in Vue tries to call .getBoundingClientRect() on a text node.

if (prevChildren) {
    for (let i = 0; i < prevChildren.length; i++) {
        const child = prevChildren[i];
        setTransitionHooks(child, resolveTransitionHooks(child, cssTransitionProps, state, instance));
        positionMap.set(child, child.el.getBoundingClientRect());
    }
}

When this code runs, prevChildren is an array with two elements, the second of which is a <div> with the key tab-content-image, but the first of which is a text node consisting of a single space (presumably the result of whitespace collapsing). There is an empty line between the </concept-chips> closing tag and the <div opening tag in App.vue, but that shouldn't matter.

These text nodes are also where the <TransitionGroup> children must be keyed warnings come from: the code sees text nodes in the transition group that don't have a key set.

I'll try upgrading to the latest version of Vue 3 to see if it's fixed there. If not, I'll try to create a minimal reproduction case, and submit a fix upstream.

This turned out to be an upstream bug in Vue that only happens in bizarrely specific circumstances (it's reproducible only in the migration build and only when client-side template compilation is used). I've filed it upstream as https://github.com/vuejs/vue-next/issues/4622 , along with a related bug that's easier to explain and reproduce.

Change 716034 abandoned by Anne Tomasevich:

[mediawiki/extensions/MachineVision@master] Update Vue init code to use Vue.createMwApp()

Reason:

In favor of 737347

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

Change 714102 merged by jenkins-bot:

[mediawiki/extensions/MediaSearch@master] Update Vue init code to use Vue.createMwApp()

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

Change 716035 merged by jenkins-bot:

[mediawiki/extensions/MediaSearch@master] Update code to unbreak use with Vue 3 migration build

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

All extensions have been tested and fixed. Thanks everyone who worked on this!

I don’t think Wikibase should be considered ready yet, at least for Termbox we still need to update the submodule in Wikibase.git if I’m not mistaken.

I don’t think Wikibase should be considered ready yet, at least for Termbox we still need to update the submodule in Wikibase.git if I’m not mistaken.

Oops, you're right. I noticed that independently when I tried to merge the MW core patch and CI failed because of Selenium tests in Termbox. I've submitted this patch to update Termbox, and that should be the last step for real this time, I hope.

The Termbox patch is now merged