| Jdlrobson | |
| Aug 3 2017, 10:31 PM |
| F9157771: view2.png | |
| Aug 23 2017, 3:51 PM |
| F9157757: view.png | |
| Aug 23 2017, 3:51 PM |
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)
var msg = mw.msg, user = mw.user, template = mw.template,
| Subject | Repo | Branch | Lines +/- | |
|---|---|---|---|---|
| Hygiene: Make usages of mw.user clearer | mediawiki/extensions/MobileFrontend | master | +22 -14 |
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | Jdlrobson | T166905 [EPIC] Talk about and improve our frontend code architecture | |||
| Declined | None | T172454 mw.template, mw.user and mw.msg should be wrapped by local methods in View.js |
We chatted about this and were tempted to decline this:
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.
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:
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.
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
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:
| type | number of files | number of mw.{msg,message} usages |
|---|---|---|
| Class-like modules | 30 | 133 |
| entry points or effectful modules | 15 | 41 |
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.
^ 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.
Change 387961 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Hygiene: Make usages of mw.user clearer
@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.
Change 387961 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: Make usages of mw.user clearer
In grooming we've decided to decline this, given the upcoming MobileFrontend.js project.