Problem:
When clicking the link to open the Bridge too soon after reloading the page I get sent to Wikidata directly. I assume this is because there is a delay between page load and Bridge code kicking in. We should see if we can reduce this or find some other way to solve the issue.
Acceptance criteria:
timeboxed: 1 day
Story time notes:
One thought I have: the init module is supposed to be small, but dist/data-bridge.init.js currently occupies some 83 KiB. Why do we send it through Babel etc. at all? If it’s not too much code, maybe we should just write it in plain JS, trading some development convenience for a drastic reduction in script size.
I did some small experiments with performance.mark(). (Helpfully, there is already an mwStartup mark emitted by the MediaWiki startup module.)
On my local wiki, it always takes ca. three seconds until even the mwStartup event. This time seems to be dominated by the network request to get the startup module (even when it’s cached, the server needs a while to generate the 304 Not Modified response), and I don’t think we can do anything about it.
If the network cache is disabled, it takes a bit more than two seconds (up to 2½) to get from mwStartup to the beginning of init.ts, and then a bit less than one second from there until the event listeners are added to the links. With network cache, the first time shrinks to about a second and the second one to ca. 100 ms; the first one is still largely due to waiting on the server to return 304 Not Modified, while the second one doesn’t seem to hit the server at all (though I don’t understand why). The time spent in our own code, e. g. to identify the links, is negligible as far as I can tell.
On Beta, custom performance marks aren’t available, but the server is generally faster. However, there’s a suspiciously long gap between the end of the network request for the init module and the beginning of the network request for the app module – slightly over half a second, with or without cache, whereas locally it’s more on the order of 0.3 s. I suppose that might be due to the larger number of things running on Beta in general – are we really running that much synchronous JS in the various modules?
Can we change the order in which the initial modules are loaded?
We probably can with some more-or-less evil hacks (register some hook to reorder [$wgResourceModules](https://www.mediawiki.org/wiki/Special:MyLanguage/Manual:$wgResourceModules), register some hook that makes a core module depend on our init module so it gets elevated in the dependency graph, …), but I’m not convinced that’s fair to other modules :)
I see two options to improve performance (independent of each other):
I’m still very tempted to rewrite it in plain JS and serve that as a regular old ResourceLoader module – according to find src/mediawiki/ -type f -name '*.ts' -exec cat {} + | wc -l it’s some 300 lines of TypeScript, that’s not the end of the world –
Correction: the init module also includes some files from src/data-access and a few others, so ca. 700 lines seems to be closer to the real number. (But that’s also including some interfaces and enums.)
but we can also first look into why it’s so bloated, and what kind of code webpack is including in that bundle.
Doing that in the separate task T228857: Step 1: minification of common.js (formerly: Investigate common.js bundle content/size).
These are not used until after loading the app module. Maybe we can also look into whether it is possible to move them there?
Good point – perhaps we can build another commonjs bundle and include that in the app ResourceLoader module.
I couldn’t figure out how to make it part of the app module, but extracting the dispatcher into a separate module reduces the size of the init module substantially:
diff --git a/client/data-bridge/package.json b/client/data-bridge/package.json index fd81fd9f4..383145e24 100644 --- a/client/data-bridge/package.json +++ b/client/data-bridge/package.json @@ -8,6 +8,7 @@ "build": "npm-run-all build:*", "build:app": "NODE_ENV=production vue-cli-service build --no-clean --target lib --formats commonjs src/main.ts", "build:init": "NODE_ENV=production vue-cli-service build --no-clean", + "build:dispatcher": "NODE_ENV=production vue-cli-service build --no-clean --target lib --formats commonjs --name dispatcher src/mediawiki/Dispatcher.ts", "fix": "vue-cli-service lint --max-warnings 0 .", "test": "npm-run-all test:*", "test:lint": "vue-cli-service lint --max-warnings 0 --no-fix . && stylelint --syntax scss 'src/**/*.(vue|scss)'", diff --git a/client/data-bridge/src/mediawiki/init.ts b/client/data-bridge/src/mediawiki/init.ts index 3d78cc33c..62516a32d 100644 --- a/client/data-bridge/src/mediawiki/init.ts +++ b/client/data-bridge/src/mediawiki/init.ts @@ -1,9 +1,10 @@ import MwWindow from '@/@types/mediawiki/MwWindow'; import BridgeDomElementsSelector from '@/mediawiki/BridgeDomElementsSelector'; import { SelectedElement } from '@/mediawiki/SelectedElement'; -import Dispatcher from '@/mediawiki/Dispatcher'; +import { default as DispatcherType } from '@/mediawiki/Dispatcher'; const APP_MODULE = 'wikibase.client.data-bridge.app'; +const DISPATCHER_MODULE = 'wikibase.client.data-bridge.dispatcher'; const WBREPO_MODULE = 'mw.config.values.wbRepo'; const FOREIGNAPI_MODULE = 'mediawiki.ForeignApi'; const ULS_MODULE = 'jquery.uls.data'; @@ -28,12 +29,14 @@ export default async (): Promise<void> => { if ( linksToOverload.length > 0 ) { const require = await mwWindow.mw.loader.using( [ APP_MODULE, + DISPATCHER_MODULE, WBREPO_MODULE, FOREIGNAPI_MODULE, ULS_MODULE, MWLANGUAGE_MODULE, ] ), app = require( APP_MODULE ), + Dispatcher: typeof DispatcherType = require( DISPATCHER_MODULE ).default, dispatcher = new Dispatcher( mwWindow, app, dataBridgeConfig ); linksToOverload.map( ( selectedElement: SelectedElement ) => { diff --git a/client/resources/Resources.php b/client/resources/Resources.php index 9c9a0e0f2..085d2bccd 100644 --- a/client/resources/Resources.php +++ b/client/resources/Resources.php @@ -66,6 +66,24 @@ } ], + 'wikibase.client.data-bridge.dispatcher' => [ + 'factory' => function () { + $clientSettings = WikibaseClient::getDefaultInstance()->getSettings(); + return new ResourceLoaderFileModule( + [ + 'scripts' => [ + 'dispatcher.common.js' + ], + 'targets' => $clientSettings->getSetting( 'dataBridgeEnabled' ) ? + [ 'desktop', 'mobile' ] : + [], + 'remoteExtPath' => 'Wikibase/client/data-bridge/dist', + ], + __DIR__ . '/../data-bridge/dist' + ); + }, + ], + 'wikibase.client.data-bridge.app' => [ 'factory' => function () { $clientSettings = WikibaseClient::getDefaultInstance()->getSettings();
$ git show @:client/data-bridge/dist/data-bridge.init.js | wc -c 85411 $ cat client/data-bridge/dist/data-bridge.init.js | wc -c 56988
The new dispatcher.common.js is much larger than this difference, but I hope that’s mostly because it’s not minified whereas data-bridge.init.js is.
Tried it out and I think this direction is promising. But I'm not sure whether this is another of those global Resource Loader modules that are currently trying to reduce or not? Though maybe the discussion about extracting the dispatcher should happen in its own (sub-) ticket?
init.js ... Why do we send it through Babel etc. at all?
trading some development convenience for a drastic reduction in script size.
We should be able to find a configuration that works for us, instead of us working around the tool chain we picked to help us.
Can we change the order in which the initial modules are loaded?
I share your skepticism about that option.
we can start that load [of app], but then immediately add our event handlers
Agree
reduce the size of the init module
The most effective way of doing this in my eyes would be to reconsider what are "app services".
Instead of passing a ready-made service container into app we could construct it inside of it,
passing it only (based on an interface not adding compile size) what is needed to do so (e.g. a ForeignApi).
As a consequence, the concrete repo implementation would all move into app without sacrificing any
conceptual clarity. What do you think?
I’m still very tempted to rewrite it in plain JS
team discussions/decisions are a thing
dispatcher into a separate [3rd] module
By definition the sum of modules can not be smaller than before.
Rather each module carries its own copy of compatibility features and the sum is hence bigger.
When ever links are detected, both dispatcher and app would be needed - never only one. What would we gain?
Is there a majority to create tickets for the two bold statements (pun intended)?
We should be able to find a configuration that works for us, instead of us working around the tool chain we picked to help us.
I’m not proposing to work around the tool chain – I’m proposing to ditch the tool chain, in this small part of the project where I believe it doesn’t carry its weight.
dispatcher into a separate [3rd] module
By definition the sum of modules can not be smaller than before.
Rather each module carries its own copy of compatibility features and the sum is hence bigger.
When ever links are detected, both dispatcher and app would be needed - never only one. What would we gain?
Reducing the size of the sum was not the goal. The goal was to address the delayed kicking in of Bridge, i. e. to make sure it’s initialized earlier. Moving the dispatcher to a separate module and thereby reducing the size of the init module (meaning less code needs to be loaded and parsed) helps with that goal, even if it increases the total code size. That is what we would gain.
I also think that, conceptually, this is not too different from your proposal to move the services into the app module. In both cases, the important part is that we move the services out of the init module.
Is there a majority to create tickets for the two bold statements (pun intended)?
The first one seems to be completely uncontroversial, so I created a task for it already: T235767: Attach event handlers to Data Bridge links as soon as possible
edit: nevermind ^^
I suggest we close this now and take on T235765 and T235771.
On completion of the latter we should take a look at the size of the init bundle and re-evaluate if further steps are needed. I also would not be surprised if T228857: Step 1: minification of common.js (formerly: Investigate common.js bundle content/size) gains us some knowledge worth applying to init as well.
I am not yet satisfied with the (lack of) discussion about my second suggestion, to stop using webpack for the init module. (We could conceivably still use TypeScript, and add the individual resulting .js files to ResourceLoader.)
I am not yet satisfied with the (lack of) discussion about my second suggestion, to stop using webpack for the init module
We should look at arguments, and compare pros and cons. So far we only have a suggestion for a change in technology but no measurable success criteria. Once we have them we can look into what is the solution with the least amount of downsides which gets us there. What is a file size for init you deem acceptable? How do you plan to ensure the browser matrix we claim to support stays intact long after this change is long forgotten, and for changes done by the less gifted (counting myself)?
ditch the tool chain, in this small part of the project where I believe it doesn’t carry its weight.
The problem I have with this is that the tool chain was chosen to satisfy certain demands (granted, we probably could have done a better job documenting them to be able to refer to them now), but "ditching" the tool chain without regard for features lost thereby at the faintest amount of problem does sound more like avoidance behavior rather than a solution - especially as we are talking about what is arguably the most popular JS build chain there is, not some random 3-star-github-repo. Having written this I regret time is invested into even considering that, rather than looking for a solution - but maybe it at least acts as a catalyst for us to collectively set ambitious (yet realistic) dist size goals and continuously keep an eye on them.
So far we only have a suggestion for a change in technology but no measurable success criteria.
From looking at the reports created during the building of our app, it seems realistic that we could get the init module down to 10-15 KB from currently >80 KB. Nonetheless, it would be nice to actually have a budget for this.
How do you plan to ensure the browser matrix we claim to support stays intact long after this change is long forgotten, and for changes done by the less gifted (counting myself)?
Would be nice to have cross-browser testing by default. That not being available, I guess that we would have to rely on the way it is done (or not done) for the rest of Wikibase as well.
arguably the most popular JS build chain there is, not some random 3-star-github-repo.
While that might be true, Mediawiki/Wikipedia is not a run-of-the-mill webapp where when you have seen one, you have seen them all. I'm not sure this build chain was designed to provide a build to be integrated into a preexisting extensive js environment. It would at least be nice not to include our own polyfills twice.
All these things being said, I'm not sure that the size of the init script has a big influence on the loading time (at least on anything faster than GPRS/Edge).