Page MenuHomePhabricator

Using WVUI via ResourceLoader should expose icons
Open, Needs TriagePublic

Description

Problem statement

Avoid needing to copy the icon code from the WVUI library to places that want to use it.

Technical details

Follow-up to T271353: Add WVUI as a vendor library in core.
There is a distribution file, commonjs2/wvui-icons.commonjs2.js, that could be used if it were provided in core
This was discussed on https://gerrit.wikimedia.org/r/c/mediawiki/core/+/641052, see https://gerrit.wikimedia.org/r/c/mediawiki/core/+/641052/19/resources/src/wvui/wvui.js

SDAW-MediaSearch has a copy of the wvui-icon component (which should probably be addressed by just using the one available from core...) and has documentation saying "To use a new icon, find the icon in src/themes/icons.ts in the WVUI library, copy the icon data, and paste it info lib/icons.js in this extension." That lib file currently has 20 copied icons, and has documentation saying "This file can be removed when Media Search uses the Icon component from the WVUI library, where icons are included." but the Icon component does *not* include the actual icon codes, those are separate
WikibaseMediaInfo has a similar copy of the wvui icon, and the same 20 icons copied from wvui
WikiLambda has another copy of the icon and the same 20 icons copied from the WikibaseMediaInfo
MediaWiki-extensions-GlobalWatchlist, which has been updated to use the wvui components from resource loader rather than needing to make a copy, still has to copy the 7 icons needed

It seems that, for the first three extensions listed, SDAW-MediaSearch copied the wvui component and icons it needed from wvui directly, WikibaseMediaInfo copied them from SDAW-MediaSearch, and WikiLambda copied them from WikibaseMediaInfo. We should avoid needing to copy these all over the place

Of note, the distribution lib file currently in core, lib/wvui/wvui.commonjs2.js, already includes all of these icons, but they are just not exported and so most are defined and never used. See "CONCATENATED MODULE: ./src/themes/icons.ts"

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

For performance reasons, we'd like to avoid exposing the entire set of icons, because it's so large. Instead, I think we'll want to offer two options:

  • Using a build step, tree-shake just the icons that are needed, and include those in the built bundle
  • Allow RL modules to list the icons they need in their module definition, and build some ResourceLoaderImageModule-style magic to automatically add wvuiIcons.iconName = '...svg string...'; to those modules

For performance reasons, we'd like to avoid exposing the entire set of icons, because it's so large. Instead, I think we'll want to offer two options:

  • Using a build step, tree-shake just the icons that are needed, and include those in the built bundle
  • Allow RL modules to list the icons they need in their module definition, and build some ResourceLoaderImageModule-style magic to automatically add wvuiIcons.iconName = '...svg string...'; to those modules

Neither one of those would work for onwiki scripts though, right?

Volker_E renamed this task from Using wvui via ResourceLoader should expose icons to Using WVUI via ResourceLoader should expose icons.Jun 15 2021, 6:48 PM

For performance reasons, we'd like to avoid exposing the entire set of icons, because it's so large. Instead, I think we'll want to offer two options:

  • Using a build step, tree-shake just the icons that are needed, and include those in the built bundle
  • Allow RL modules to list the icons they need in their module definition, and build some ResourceLoaderImageModule-style magic to automatically add wvuiIcons.iconName = '...svg string...'; to those modules

Neither one of those would work for onwiki scripts though, right?

The latter could work for gadgets if we made the syntax simple enough and supported it in gadget definitions. For user scripts it wouldn't work, and we may want to keep a (deprecated?) wvui-icons module around for that use case.

For performance reasons, we'd like to avoid exposing the entire set of icons, because it's so large. Instead, I think we'll want to offer two options:

  • Using a build step, tree-shake just the icons that are needed, and include those in the built bundle
  • Allow RL modules to list the icons they need in their module definition, and build some ResourceLoaderImageModule-style magic to automatically add wvuiIcons.iconName = '...svg string...'; to those modules

Neither one of those would work for onwiki scripts though, right?

The latter could work for gadgets if we made the syntax simple enough and supported it in gadget definitions. For user scripts it wouldn't work, and we may want to keep a (deprecated?) wvui-icons module around for that use case.

So I was look at why the dist files for the main wvui and wvui-search modules already include the icons anyway, and I think its because in Input.vue there is an import of wvuiIconClear from the icons file. Given that the icon data is already present in the dist files, would adding it to the exports result in needing to repeat the data again? Or just move it around so that its added to the exports with minimal increase in bundle size?

A quick test later found that adding all of the icons to the default exports of the wvui and wvui-search bundles would increase each by 22KB, compared to the 91KB of the dedicated wvui-icons module (sizes are based on the commonjs2 distribution). But, we shouldn't add to the defaults, because then we can't pass the default exports directly to the components property of our app. Instead, maybe add to the WvuiIcon component the ability to just be given the name of our icon, and if it matches one of the icons in the library it would then use that, like OOUI does.

So I was look at why the dist files for the main wvui and wvui-search modules already include the icons anyway, and I think its because in Input.vue there is an import of wvuiIconClear from the icons file. Given that the icon data is already present in the dist files, would adding it to the exports result in needing to repeat the data again? Or just move it around so that its added to the exports with minimal increase in bundle size?

This happens because dead code elimination is currently broken in our builds (possibly because minification is disabled for the commonjs2 builds). Once we fix this issue, then only wvuiIconClear will be included, and the other icons (that aren't used by Input.vue) will not be, which will significantly reduce the size of the icon-less build.

A quick test later found that adding all of the icons to the default exports of the wvui and wvui-search bundles would increase each by 22KB, compared to the 91KB of the dedicated wvui-icons module (sizes are based on the commonjs2 distribution). But, we shouldn't add to the defaults, because then we can't pass the default exports directly to the components property of our app. Instead, maybe add to the WvuiIcon component the ability to just be given the name of our icon, and if it matches one of the icons in the library it would then use that, like OOUI does.

I think that 22KB is all export code (repeating __webpack_require__.d(icons_namespaceObject, "wvuiIconAlert", function() { return wvuiIconAlert; }); for every icon adds up to a lot), which is another way in which our current build is inefficient. We want to move WVUI away from Webpack to Rollup or Vite in the near future, which would fix both of these things (dead code elimination and super high export overhead).