Page MenuHomePhabricator

mw.template, mw.user and mw.msg should be wrapped by local methods in View.js
Closed, DeclinedPublic

Description

We discussed this during our code review session on 3rd August 2017.

Problem statement:
Currently by using mw.msg, mw.user and mw.template we make it difficult to easily see usages (as we don't use dependency injection)

Current proposal

  • Where mw.user is used we will add a variable to the top of the file to make it clearer that these are dependencies and being used
  • Do same for mw.msg
  • Do same for mw.template
var msg = mw.msg,
  user = mw.user,
  template = mw.template,

Event Timeline

Jdlrobson moved this task from Incoming to Upcoming on the Readers-Web-Backlog board.

We chatted about this and were tempted to decline this:

  • There were concerns that mw.msg would still be used outside View e.g. in startup/init modules
  • There were concerns that there was not so much benefit for this and that the same impact could be had with the use of an alias.
  • This would break the resourcemodules linter

This seems equivalent to using the globals to me.

If we do the resource-modules linter won't be able to check the messages and templates used in code against extension.json (unless we do extra work for the linter to support these constructs).

@bmansurov mentioned also that if we put this in View then other code like entry points or else won't have that utility and will have to use the global function too.

bmansurov added a comment.EditedAug 8 2017, 4:34 PM

Would it mean we'd have to have two different ways of getting messages depending on where we're doing so? For example, from View and subclasses we'd use the proposed method, but from everywhere else we'd use mw.msg.

Edit: The above two comments summarized my point well. We just commented at the same time. ;)

If the goal is to show clearer where these come from hoisting an alias to the top may be more appropriate rather than pass them in as dependencies.
e.g.

mwMsg = mw.msg,

@pmiazga what do you think?

I'm against globals, if we can get rid of those count me in. Having msg() or getTemplate() inside View sounds tempting. With this approach, we won't have to include those globals in every file but the cost seems bit high (issues with ResourceModule linter, adding another strict dependency between View and mw.xxxx).

What about having a "templating" object that would be used in View and to other places that do not use View. This will give us a boundary between MW and our extensions, easier testing (no need to stub mediaWiki variable), This templating (let's call it templating for now as nothing other comes to my mind) for would have msg(), getTemplate() for now, we could move more things there if necessary.

What about having a "templating" object that would be used in View and to other places that do not use View.

Only Views use the template object.

So what is the value statement?
Is this about making things easier to test? making it clearer about what our code is using or something else?
I think this will guide us. Right now I'm not seeing a pay off relative to the work proposed....

To me: the value of this task is to stop using globals and pass all required dependencies to the class. It will make tests easier and make classes more explicit.When
When msg and getTemplate get implemented inside View - everything what extends View will not use global state directly, only use the interface provided by View. With that approach, we will reduce extension/mw core "connections" to a minimum.

What we have right now:


After this change we will have:

As you see the second image looks bit cleaner (fewer interactions between different elements in the graph)

Honest question: Isn't that just a smoke curtain? You are still using globals and now you have an abstraction different to your code not in Views, core and all extensions about how you go fetch labels.

pmiazga added a comment.EditedAug 23 2017, 7:50 PM

It can look like a smoke curtain but it's shifting the responsibility how to build text messages/templates from every class to main the View class. Under the hood we will still call the global but

  • concrete classes inside MobileFormatter use built-in methods instead of asking mw.xxx
  • things that extend the view do not depend upon mw.msg anymore, it's up to View how to define msg() or getTemplate() methods

My rule of thumb: First - only boundary classes can call methods from different extensions or core. Everything other has to use what given extension provides, second: keep the number of boundary classes as small as possible.

Edit:
The real question is, do we want to treat every extension as a separate plug-in or is it something that we don't care and MWCore and Extensions can be tangled together.

My concern is that then we will have another different pattern to deal with i18n that also relies on a global but just hides it.

Not all files are classes that inherit from View. See for example, from MobileFrontend and MinervaNeue, using mw.msg and mw.message there are:

typenumber of filesnumber of mw.{msg,message} usages
Class-like modules30133
entry points or effectful modules1541

Just for information, 33% of the files, with 24% of the usages of mw.{msg,message}.


Personally, I've gotten to see mw.msg like something you import at the top of your file. Something like:

var msg = require('mw.msg')

After a lot of time, I've internalized that it is better to have 1 consistent pattern, rather than 2 different ones, when they perform the same functionality.


I think there is are some interesting ideas in the proposal, but I'm not sure hardcoding a global access in one of the super classes is the best solution.

I'd like to hear the thoughts from others, I don't want to be a blocker for the effort, just thought I'd chime in.

Source for data: ag "mw.message" --js -l -c and ag "mw.msg" --js -l -c on MobileFrontend and MinervaNeue.

Files then filtered and partitioned by /\v\/[^A-Z][^\/]+\.js, and manually removed the test files.

Jdlrobson updated the task description. (Show Details)Aug 24 2017, 2:10 PM

^ How do we feel about this proposal?

@Jhernandez is right, we do use these outside the View, although the use of template outside is very rare and we might want to discourage that, in which case a View.prototype.template method is not that bad an idea. I agree however that msg usage could be made more obvious at the top of the file, if the goal is to make it clearer what dependencies are being used.

Maybe we need to start with a small incremental change like the proposal and see where that gets us?

In my opinion that isn't a big win and I wouldn't do it, but I'm not against it if others want to do it. If we do it, let's be sure that all the code base is consistent so that we can reliably grep for message or template usages.

Sounds good to me. Explicit dependencies definition on top of the file is a good craftsmanship. I think the Popups codebase is already sticking to that rule, every file has a set of imports as the first thing in each file.
I agree with @Jhernandez, if we want to go with it, all occurrences have to be refactored to follow only one style.

Jdlrobson lowered the priority of this task from High to Low.Sep 6 2017, 8:04 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a project: patch-welcome.

Let's make this low but it sounds like we're happy with the proposal.

Jdlrobson renamed this task from mw.template and mw.msg should be wrapped by local methods in View.js to mw.template, mw.user and mw.msg should be wrapped by local methods in View.js.Oct 31 2017, 11:52 PM
Jdlrobson added a project: User-Jdlrobson.
Jdlrobson updated the task description. (Show Details)
Jdlrobson added subscribers: Jrbranaa, gerritbot.
Jdlrobson moved this task from Inbox to Next up on the User-Jdlrobson board.Nov 1 2017, 6:29 PM

Change 387961 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Hygiene: Make usages of mw.user clearer

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

@pmiazga if you are still interested in doing this I think the easiest way to do this would be to pair program and get this done. Shouldn't take long. The above takes care of mw.user which is relatively straightforward.

Jdlrobson moved this task from Next up to Focus on the User-Jdlrobson board.Nov 1 2017, 11:28 PM
Jdlrobson moved this task from Focus to Next up on the User-Jdlrobson board.Nov 13 2017, 5:52 PM
Jdlrobson raised the priority of this task from Low to Normal.Feb 24 2018, 12:13 AM

Change 387961 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: Make usages of mw.user clearer

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

Jdlrobson updated the task description. (Show Details)
Jdlrobson closed this task as Declined.Mar 13 2018, 4:42 PM

In grooming we've decided to decline this, given the upcoming MobileFrontend.js project.