Page MenuHomePhabricator

Create a temporary entry for all WVUI components
Closed, ResolvedPublic

Description

For reasons noted by @Jdlrobson in this comment, as we build out WVUI, we need to avoid adding extraneous code to the bundle used in Vector to build the typeahead search feature. Eventually, we'll create a special entry for Vector containing only those components needed, and eventually we could mitigate this kind of thing altogether with tree-shaking, but during the current phase of experimentation with WVUI, we will just create a separate entry point that contains all components, so that the existing wvui entry remains untouched.

Event Timeline

Before we start making changes, I want to make sure I 100% understand what is going on in the current setup. Here is what we are currently generating in terms of output when the WVUI build script is run:

First, there is the CommonJS bundle for ResourceLoader:

  • dist/commonjs2/wvui.commonjs2.js (133KB)
    • Bundled code for all JS components, unminified
    • CommonsJS format (module.exports, require, etc) for compatibility with ResourceLoader
    • includes certain icon paths inlined as strings
    • does not include CSS
  • dist/commonjs2/wvui-icons.commonjs2.js (93KB)
    • Same as above, but only contains icon path data
    • Unminified
  • dist/commonjs2/wvui.css (12KB)
    • Unminified CSS for all components (RL handles minification)

In addition to the CommonJS build, there is an aggressively minified set of files with accompanying source maps living in the top level of the /dist folder:

  • dist/wvui.js (20KB)
    • Bundled, minified code for all JS components in UMD format
    • Includes inlined SVG icon for the search icon only (anything else)?, does not include the full icon set
  • dist/wvui-icons.js (67KB)
    • UMD format, minified bundle for the complete set of icon paths as strings
  • dist/wvui.css (10KB)
    • minified CSS for all components
  • dist/wvui.css.map, dist/wvui.js.map.json, dist/wvui-icons.js.map.json
    • Source maps for the above minified files
  • dist/src
    • The actual source files that the source maps reference

Correct me if I'm wrong, but no one is currently consuming WVUI through the second set of files, correct? I assume we are generating these to support non-MediaWiki usage of the library at some point in the future. Does anyone know if the minification that MediaWiki performs on the CommonsJS build of WVUI brings the files to a comparable size as the optimized Webpack build here?

As far as next steps, I propose that we do the following:

  • Create a new wvui-vector endpoint that bundles only what is needed for Vector search; this bundle only needs to be produced in CommonJS format for ResourceLoader since it is MW-specific
  • Keep the default wvui entry point unchanged for now; eventually as we add more components they will start to show up here. This code will be bundled in both UMD and CommonJS formats same as before.
  • Update the Resources.php file in core to include two separate modules: wvui-vector and wvui (everything); a corresponding patch in Vector will be needed at the time of the name change

Order of operations would be:

  1. Create the new entry point in WVUI
  2. Update the Resources.php definitions (wvui-vector and wvui as separate modules)
  3. Update Vector to require( 'wvui-vector' ) at the same time as 2
  4. Start adding new components to the main WVUI endpoint, leaving wvui-vector alone

Does this sound correct? If so I can open a Webpack patch in WVUI later this week to introduce the second entry point and adjust some of the build settings.

Change 682763 had a related patch set uploaded (by Eric Gardner; author: Eric Gardner):

[wvui@master] Create a dedicated "wvui-vector" entry point and bundle

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

Everything you asked about is correct, as far as I know

Change 682763 merged by jenkins-bot:

[wvui@master] [build] Create a dedicated "wvui-search" entry point and bundle

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

Catrope claimed this task.
Catrope reassigned this task from Catrope to egardner.

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

[mediawiki/core@master] WVUI: Create new wvui-search module

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

Change 697859 merged by jenkins-bot:

[mediawiki/core@master] WVUI: Create new wvui-search module

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

Change 697819 had a related patch set uploaded (by DannyS712; author: DannyS712):

[mediawiki/core@master] Restore package exports of 'wvui' resource loader module

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

DannyS712 subscribed.

Having the 'wvui' module just depend on 'wvui-search' meant that it was no longer providing the package exports for callers, which breaks things (like GlobalWatchlist) - patch sent to restore the exports

thanks for flagging @DannyS712 , will take a look tomorrow with @Catrope

Change 697819 merged by jenkins-bot:

[mediawiki/core@master] Restore package exports of 'wvui' resource loader module

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

Change 698298 had a related patch set uploaded (by DannyS712; author: DannyS712):

[mediawiki/core@master] Add 'mobile' as a target for wvui ResourceLoader module

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

Change 698298 merged by jenkins-bot:

[mediawiki/core@master] Add 'mobile' as a target for wvui ResourceLoader module

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

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

[mediawiki/core@master] wvui: Change the wvui module back to the full WVUI bundle

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

As @Jdlrobson points out on the patch, having a full wvui build and a wvui-search build causes double-loading in some cases (e.g. when loading the search on a page that has QuickSurveys on it). We should think about how we want to address that.

One way could be to provide an "everything but search" build that can be loaded on top of the search build to make the full library. Then the wvui module could contain this build and depend on wvui-search.

Another way we could (partly) deal with this is by adding a skipFunction to the wvui-search module so that it doesn't load when the wvui module is already loaded, but that wouldn't solve anything if wvui-search is loaded first and wvui is loaded second, or if both are loaded at the same time.

One way could be to provide an "everything but search" build that can be loaded on top of the search build to make the full library. Then the wvui module could contain this build and depend on wvui-search.

Yeh. Code splitting seems like the right approach here. https://webpack.js.org/configuration/optimization/#optimizationsplitchunks

  • wvui-search would contain the common JS (which presumably is all the search stuff)
  • wvui requires wvui-search, adds the additional stuff.

We do something similar in MobileFrontend with webpack (I'm not sure what the equivalent is in Rollup.js)

Another way we could (partly) deal with this is by adding a skipFunction to the wvui-search

Seems like a short term fix. Is it worth the complexity? I'd suggest simply double loading in the interim stage.

Change 699951 merged by jenkins-bot:

[mediawiki/core@master] wvui: Change the 'wvui' module back to the full WVUI bundle

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