Page MenuHomePhabricator

VueComponentParser doesn't support components using render functions instead of templates
Closed, DeclinedPublic

Description

Steps to reproduce:
Try to use a Vue component for a file name ending in .vue that has a <script> tag with module.exports providing a render function, and no <template> tag

Expected result:
VueComponentParser supports components with render functions

Actual result:
VueComponentParser complains about the missing template

While this could be worked around by having the file name not end in .vue, that shouldn't be needed as a work around

Event Timeline

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

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

[mediawiki/core@master] VueComponentParser: support components using render functions

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

I think its more likely that not having a template is a mistake or accident than it being intentional. Renaming to .js seems like a reasonable way to make that work, and has the benefit of steering people in the right direction instead of no longer having mistake-detection for forgetting or breaking <template>. It also seems valuable for performance long-term to encourage use of templates and not JS-heavy render functions.

If we do accept this, I think it should be paired with a document use case for when and why a feature is better off with a render function in some cases.

I think its more likely that not having a template is a mistake or accident than it being intentional. Renaming to .js seems like a reasonable way to make that work, and has the benefit of steering people in the right direction instead of no longer having mistake-detection for forgetting or breaking <template>. It also seems valuable for performance long-term to encourage use of templates and not JS-heavy render functions.

If we do accept this, I think it should be paired with a document use case for when and why a feature is better off with a render function in some cases.

As for use case: I discovered this while preparing to add a component with a render function to GlobalWatchlist for T260220.
Renaming to .js can result in confusing because require( 'component.vue' ) is clearly a Vue component that can be used in an app, but require( 'component.js' ) might be utilities for that component, or similar. It also means that our eslint rules for vue components won't be triggered.
Perhaps to maintain the warnings for leaving out <template> by mistake, we can check if the script includes render: somewhere in it, which suggests that a render function is being used.

I don't see anything in T260220 that warrants using specifically using Vue, and using it with a render function. (There's also no patch attached yet.) Why are you doing that?

I don't see anything in T260220 that warrants using specifically using Vue, and using it with a render function. (There's also no patch attached yet.) Why are you doing that?

In global watchlist, currently the log entry for a page being deleted is just "Log: delete/delete <the page> (user: comment)" and its similar for all log entries - we just show the log type and action, not an actual message for what happened. My goal was to change that to use the existing log messages.
For page deletion, the log message should be logentry-delete-delete, which is (for English) "$1 {{GENDER:$2|deleted}} page $3". $3 is a variable for the page, and it includes a link to the page, a trigger to unwatch (or rewatch) the page, and some other stuff.
We can't just use the mw.msg() / mw.message() / new mw.Message() system, because the variables we want to substitute are not strings. Accordingly, I was going to add custom handling (described in more details at T260220#7267916) to transform "$1 {{GENDER:$2|deleted}} page $3" into (for the Vue version of the display) [ <a VNode for the user $1>, ' deleted page ', <a VNode for the page $3> ]. These would then be passed as child elements of the overall entry, via a render function

render: function ( createElement ) {
	// Extremely simplified
	var childNodes = someLogEntryManager.method();
	return createElement(
		'li',
		childNodes
	);
}

which would allow maintaining the event handlers and bindings for the links, etc. that are included.

I'm no Vue expect, but I would expect that to be possible in <template> just the same. There's presumably a way to embed a JavaScript expression, assign it to the varialbe like you have here, and then place that variable within a <li> element. If other developers or upstream help channels say that it's not possible, then I think I'd agree that that seems like an important thing to support.

I'm no Vue expect, but I would expect that to be possible in <template> just the same. There's presumably a way to embed a JavaScript expression, assign it to the varialbe like you have here, and then place that variable within a <li> element. If other developers or upstream help channels say that it's not possible, then I think I'd agree that that seems like an important thing to support.

It probably is possible, but to be able to support GENDER and PLURAL parsing, plus log entries with a bunch more variables, it would get really complicated really fast, and have a bunch of conditional nodes for each different possible reuse of each variable from $4 and on, because different log entries can use that for different things (move logs like logentry-move-move use it for the target page, logentry-delete-restore uses it for the information about what was restored, block logs like logentry-block-block use it for the gender of the target, protection logs like logentry-protect-protect use it for details about the configured protection, etc)

Using Message methods to process GENDER and PLURAL parsing is fine and all. But, how does that relate to whether the concatenation happens in a function vs in a Vue <template>? What does the code you tried look like that didn't work? Where did you get stuck? Or what error message did you get when developing locally and trying it when it didn't work?

Krinkle triaged this task as Medium priority.Aug 24 2021, 6:29 PM
Krinkle moved this task from Inbox to Backlog on the MediaWiki-ResourceLoader board.
Krinkle edited projects, added Performance-Team (Radar); removed Performance-Team.
Krinkle moved this task from Limbo to Watching on the Performance-Team (Radar) board.

Using Message methods to process GENDER and PLURAL parsing is fine and all. But, how does that relate to whether the concatenation happens in a function vs in a Vue <template>? What does the code you tried look like that didn't work? Where did you get stuck? Or what error message did you get when developing locally and trying it when it didn't work?

I didn't create a version of the component without a render function, because I figured a render function would be easier. But, it would look something like the following, to be able to handle messages with $1, $3, and $4 (not sure if there are messages with more links):

<template>
	<li>
		<span v-if="preText">{{ preText }}</span>
		<span v-if="firstVarUserDisplay" v-html="entry.userDisplay"></span>
		<span v-else-if="firstVarOtherLink">
			<a v-bind:href="firstVarLinkTarget" target="_blank">{{ firstVarLinkDisplay }}</a>
		</span>
		<span v-else> <!-- Not the user $1 or some other link $4, should be link to main target page $3 -->
			<!-- All of the stuff for the page display links -->
			<a v-bind:href="pageLink" target="_blank" >{{ entry.titleMsg }}</a>
			(<!--
			--><a v-bind:href="logPageLink" target="_blank">{{ $i18n( 'globalwatchlist-log-page' ).text() }}</a>,
			<a v-bind:href="logEntryLink" target="_blank">{{ $i18n( 'globalwatchlist-log-entry' ).text() }}</a>,
			<a v-if="pagewatched" v-on:click="unwatchPage">{{ $i18n( 'globalwatchlist-unwatch' ).text() }}</a>
			<a v-else v-on:click="rewatchPage">{{ $i18n( 'globalwatchlist-rewatch' ).text() }}</a><!--
			-->)
		</span>

		<span v-if="textAfterFirstVar">{{ textAfterFirstVar }}</span>

		<span v-if="secondVarUserDisplay" v-html="entry.userDisplay"></span>
		<span v-else-if="secondVarOtherLink">
			<a v-bind:href="secondVarLinkTarget" target="_blank">{{ secondVarLinkDisplay }}</a>
		</span>
		<span v-else> <!-- Not the user $1 or some other link $4, should be link to main target page $3 -->
			<!-- All of the stuff for the page display links -->
			<a v-bind:href="pageLink" target="_blank" >{{ entry.titleMsg }}</a>
			(<!--
			--><a v-bind:href="logPageLink" target="_blank">{{ $i18n( 'globalwatchlist-log-page' ).text() }}</a>,
			<a v-bind:href="logEntryLink" target="_blank">{{ $i18n( 'globalwatchlist-log-entry' ).text() }}</a>,
			<a v-if="pagewatched" v-on:click="unwatchPage">{{ $i18n( 'globalwatchlist-unwatch' ).text() }}</a>
			<a v-else v-on:click="rewatchPage">{{ $i18n( 'globalwatchlist-rewatch' ).text() }}</a><!--
			-->)
		</span>

		<span v-if="textAfterSecondVar">{{ textAfterSecondVar }}</span>

		<span v-if="thirdVarUserDisplay" v-html="entry.userDisplay"></span>
		<span v-else-if="thirdVarOtherLink">
			<a v-bind:href="thirdVarLinkTarget" target="_blank">{{ thirdVarLinkDisplay }}</a>
		</span>
		<span v-else> <!-- Not the user $1 or some other link $4, should be link to main target page $3 -->
			<!-- All of the stuff for the page display links -->
			<a v-bind:href="pageLink" target="_blank" >{{ entry.titleMsg }}</a>
			(<!--
			--><a v-bind:href="logPageLink" target="_blank">{{ $i18n( 'globalwatchlist-log-page' ).text() }}</a>,
			<a v-bind:href="logEntryLink" target="_blank">{{ $i18n( 'globalwatchlist-log-entry' ).text() }}</a>,
			<a v-if="pagewatched" v-on:click="unwatchPage">{{ $i18n( 'globalwatchlist-unwatch' ).text() }}</a>
			<a v-else v-on:click="rewatchPage">{{ $i18n( 'globalwatchlist-rewatch' ).text() }}</a><!--
			-->)
		</span>

		<span v-if="postText">{{ postText }}</span>
	</li>
</template>

It would work, its just a lot more complicated and harder to understand.

You can see a preliminary version of the Vue file than needs a render function at https://gerrit.wikimedia.org/g/mediawiki/extensions/GlobalWatchlist/+/refs/changes/68/714668/33/modules/vue/EntryLogWithDetails.vue#41

The heart of the difference with regards to localisation, I don't see any difference there. They both just call a function the same as otherwise, in the same place for the same purpose.

// Render
createElement( 'a', { attrs: {
		href: this.linker.linkQuery( 'title=Special:Log&logid=' + this.entry.logid ),
		target: '_blank'
	} },
	this.$i18n( 'globalwatchlist-log-entry' ).text()
);

// compared to

// Vue
<a :href="this.linker.linkQuery( 'title=Special:Log&logid=' + this.entry.logid )" target="_blank">{{ $i18n( 'globalwatchlist-log-entry' ).text() }}</a>

I would imagine most people would prefer Vue, given the choice.

Your code using Vue seems to have a lot of conditions and bindings. I don't know if that's the way you prefer it. But, if you find those complicated, then those use thos – same as in your render version.

In either case, I don't recognise any problem or difficulty here. I do recognise that there seems to be a mismatch between how you do it with Vue vs without, so I suspect it's just a case of needing to test a bit more to do it the same simple way with Vue. Remember that the Vue code literally translates to a render function internally, so whatever you do it one, you should be able to do the exact same way and simpler/shorter with Vue.

I suggest asking around for help if it seems like should is overly complicated. In my experience, it usually turns out there is a simpler way in that case.

The heart of the difference with regards to localisation, I don't see any difference there. They both just call a function the same as otherwise, in the same place for the same purpose.

// Render
createElement( 'a', { attrs: {
		href: this.linker.linkQuery( 'title=Special:Log&logid=' + this.entry.logid ),
		target: '_blank'
	} },
	this.$i18n( 'globalwatchlist-log-entry' ).text()
);

// compared to

// Vue
<a :href="this.linker.linkQuery( 'title=Special:Log&logid=' + this.entry.logid )" target="_blank">{{ $i18n( 'globalwatchlist-log-entry' ).text() }}</a>

I would imagine most people would prefer Vue, given the choice.

Your code using Vue seems to have a lot of conditions and bindings. I don't know if that's the way you prefer it. But, if you find those complicated, then those use thos – same as in your render version.

In either case, I don't recognise any problem or difficulty here. I do recognise that there seems to be a mismatch between how you do it with Vue vs without, so I suspect it's just a case of needing to test a bit more to do it the same simple way with Vue. Remember that the Vue code literally translates to a render function internally, so whatever you do it one, you should be able to do the exact same way and simpler/shorter with Vue.

I suggest asking around for help if it seems like should is overly complicated. In my experience, it usually turns out there is a simpler way in that case.

The inlining doesn't work for everything though. I pass the createElement function around to allow shared code to create, eg, the links to pages that a user is partially blocked from. But if its been decided that we won't support .vue files without a <template>, I guess I can use the earlier suggestion of just renaming the file to .js

You can also add local variables and functions to a Vue component to reduce duplication, and sub components for re-usability. The shorter syntax and explicit visibility of where elements are created is intentional. As said, I believe this to be a question about how to use Vue, and not a question of to avoid it. I think there's a lot more than we've used so far and I suggest asking around to get help from people that have worked more with Vue. Ideally the person working on the extension with you would be the person to ask such questions.