Page MenuHomePhabricator

Organise module files in directories based on their module name
Open, MediumPublic

Description

Rationale

This is a bit of clean-up for a couple reasons. (The below has since been documented in coding conventions and performance guidelines.

Finding files

Make it easier to find files in the repository based on their module name. This applies to features like "Find File" in text editors (eg. +P / +P in Atom and Sublime Text), as well as to repository viewers (eg. T on GitHub).

Contributing

Even in our current core stack (with good ol' ResourceLoader concatenating files), there's a fair amount of complexity that can overwhelm new contributors. And even for experienced contributors, we don't all work with all files all the time. So being able to easily tell which (other) files load as part of a given module is quite useful.

I would expect this to reduce overall confusion, but also reduce risk of forgetting to declare dependencies between related code paths (given files in the same directory but part of separate modules). It would also make it easier to create something in less feedback cycles of "try this", "get an error" (eg. for cases where we do detect something is broken, missing dependencies tends to not be one of them, though).

Performance

Quantify the cost of modules through visibility in the directory structure, hopefully making it easier and clearer how and where to add new code to the same module, and to discover which code is and isn't already included in the same bundle.

Past

Some past work has been put in towards this, but so far only in certain sub-trees. Never across the board.

Some past conversions in resources/src/

Also, we've been doing fairly well in resources/lib/ where every module has its own directory now, yay!

Future

This will also get things in a better shape of the future that awaits us in T133462, where it'll likely become a prerequisite for a module to either be one file, or be its own directory. Even if we don't enforce that, it'd be even more confusing than today if we have files importing each other using require('./example'); but also have files in the same directory that aren't reachable as part of the module.

Proposal

The proposal is the following two mandates:

Every file shipped in a ResourceLoader module must be in resources/ in a (sub) directory named after the module , or as a single file containing the module name in its file name.
There must not be files in a module directory that are part of a different module.
Currently fine as-is
  • "jquery.chosen" is at resources/lib/jquery.chosen/
  • "moment" is at resources/lib/moment/
  • "qunit" is at resources/lib/qunitjs/
  • "jquery.tipsy" is at resources/src/jquery.tipsy/
  • "mediawiki.toolbar" is at resources/src/mediawiki.toolbar/
  • "mediawiki.widgets.datetime" is at resources/src/mediawiki.widgets.datetime/
  • "startup" is at resources/src/startup.js
Examples to change
  • resources/lib/jquery/

This directory contains things loosely related to "jQuery" in some way or another, registered as multiple different modules. The ones that are single-file modules could be moved up to be in lib/ directory. The others should get their own directory. Leaving jquery/ to only contain jquery.js and jquery.migrate.js – the contents of the module currently registered as "jquery".

  • resources/src/mediawiki/
    • api.js
    • api/
      • category.js
    • htmlform/
    • page/
      • ready.js
      • startup.js
      • watch.js

This the "api" and "page" are remnants of my attempt to make the directory structure reflect the run-time class hierarchy (instead of the module/bundle structure). This is also predates things like module.exports.

The idea was to roughly be able to translate a public identifier in JavaScript (e.g. mw.Api.prototype.isCategory or mw.page.watch.updateLinks()) to a path on the file system. I didn't think about this well, and I regret doing it. It didn't work because the way we namespace our modules is by module name, not my namespace hierarchy. Even in the above examples we can see a few clear flaws and limitations:

  • mediawiki/api/category.js does not translate to mw.api.category = {}. Rather it translates to the module name mediawiki.api.category. The actual identifier is mw.Api and several methods such as mw.Api#isCategory.
  • mediawiki/page/watch.js - This was at some point the first and only module to justify my (bad) idea. We did have mw.page.watch. It existed. But it doesn't exist anymore. And the other files in the directory mostly don't have any public identifiers. They're just run-time logic with their file reflecting the module names: mediawiki.page.startup, mediawiki.page.ready, etc.

Given that all of these are single-file modules, there are two ways we could solve them:

  1. Put them directly in resources/src/ as mediawiki.api.category.js and mediawiki.page.ready.js.
  2. Give some of the related modules their own shared directory, e.g. resources/src/mediawiki.api/ and resources/src/mediawiki.page/, with the single-file modules living inside of it.

Both of these would satisfy the two proposed mandates.

Progress

Below listing is based on the structure as it was when we started.

Reference: https://gerrit.wikimedia.org/g/mediawiki/core/+/42956b99b558294e5aa8c6c0e2a8f18513478022/resources/

Items are checked off when the file or directory has been checked recursively to match the above two principles (and renamed or split as needed).

resources/lib/

  • CLDRPluralRuleParser/
  • html5shiv/
  • jquery/
  • jquery.chosen/
  • jquery.client/
  • jquery.i18n/
  • jquery.ui/ – T219604
  • moment/
  • mustache/
  • oojs-router/
  • oojs-ui/
  • oojs/
  • qunitjs/
  • sinonjs/

resources/src/

  • jquery.tipsy/
  • jquery/
    • jquery.color
    • jquery.confirmable
    • jquery.hidpi (removed)
    • jquery.lengthLength
    • jquery.makeCollapsible
    • jquery.farbtastic
    • jquery.tablesorter
    • jquery.spinner
    • (misc)
  • mediawiki.action/
  • mediawiki.hidpi-skip.js
  • mediawiki.language/
  • mediawiki.legacy/
  • mediawiki.less/ (less importdir: not a module)
  • mediawiki.libs.jpegmeta/
  • mediawiki.libs.pluralruleparser/
  • mediawiki.messagePoster/
  • mediawiki.rcfilters/
  • mediawiki.router/
  • mediawiki.skinning/
  • mediawiki.special/
  • mediawiki.toolbar/
  • mediawiki.ui/
  • mediawiki.widgets.datetime/
  • mediawiki.widgets.visibleLengthLimit/
  • mediawiki.widgets/
  • mediawiki/
    • mediawiki/api/
    • mediawiki/htmlform/
    • mediawiki/page/
  • moment/
  • startup.js

Follow-up

  • Consider merging 'mediawiki.ForeignApi' and 'mediawiki.ForeignApi.core.'.
  • Consider merging 'mediawiki.page.ready' and 'mediawiki.page.startup'.

See T192623 further about these.

Element.js
// FIXME: mw.htmlform.Checker also sets this to empty object
mw.htmlform = {};
Checker.js
// FIXME: mw.htmlform.Element also sets this to empty object
mw.htmlform = {};

Details

SubjectRepoBranchLines +/-
mediawiki/coremaster+3 -3
mediawiki/coremaster+23 -21
mediawiki/coremaster+8 -8
mediawiki/coremaster+29 -29
mediawiki/coremaster+2 -2
mediawiki/coremaster+8 -8
mediawiki/extensions/UploadWizardmaster+19 -12
mediawiki/coremaster+3 -3
mediawiki/extensions/CheckUsermaster+29 -19
mediawiki/extensions/TemplateDatamaster+28 -23
mediawiki/extensions/WikimediaEventsmaster+10 -10
mediawiki/coremaster+1 -1
mediawiki/coremaster+23 -25
mediawiki/extensions/CentralNoticemaster+5 -1
mediawiki/extensions/CentralNoticemaster+5 -1
mediawiki/coremaster+1 -1
mediawiki/coremaster+9 -9
mediawiki/extensions/EventLoggingmaster+6 -6
mediawiki/extensions/MultimediaViewermaster+32 -32
mediawiki/coremaster+1 -349
mediawiki/coremaster+2 -2
mediawiki/coremaster+55 -54
mediawiki/coremaster+41 -42
mediawiki/coremaster+15 -13
mediawiki/coremaster+2 -2
mediawiki/coremaster+31 -31
mediawiki/coremaster+12 -12
mediawiki/coremaster+16 -26
mediawiki/coremaster+5 -4
mediawiki/coremaster+3 -3
Show related patches Customize query in gerrit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Krinkle renamed this task from 9mdaaaaaaa to Organise module files in directories based on their module name.Jul 1 2018, 10:58 PM
Krinkle lowered the priority of this task from High to Medium.
Krinkle updated the task description. (Show Details)
Krinkle added a subscriber: Aklapper.

Change 443340 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/MultimediaViewer@master] Move files to their own directory per module

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

Change 443340 merged by jenkins-bot:
[mediawiki/extensions/MultimediaViewer@master] Move files to their own directory per module

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

Change 450598 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/EventLogging@master] Move files to their own directory per module

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

Change 450598 merged by jenkins-bot:
[mediawiki/extensions/EventLogging@master] Move files to their own directory per module

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

Change 453876 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resources: Move non-jquery files from /resources/lib/jquery to /resources/lib

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

Change 453876 merged by jenkins-bot:
[mediawiki/core@master] resources: Move non-jquery files from /resources/lib/jquery to /resources/lib

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

Change 494047 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] mediawiki.cldr: Move file to its own directory

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

Change 494047 merged by jenkins-bot:
[mediawiki/core@master] mediawiki.cldr: Move file to its own directory

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

Change 506232 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/CentralNotice@master] ext.centralNotice.startUp: Convert to packageFiles module

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

Change 506232 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] ext.centralNotice.startUp: Convert to packageFiles module

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

Change 510584 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/CentralNotice@master] ext.centralNotice.kvStore: Move to its own directory

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

Change 510584 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] ext.centralNotice.kvStore: Move to its own directory

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

Change 517109 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] specials: Combine small miscellaneous 'mediawiki.special.*' modules

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

Change 517109 merged by jenkins-bot:
[mediawiki/core@master] specials: Combine small miscellaneous 'mediawiki.special.*' modules

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

Change 542640 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] jquery.lengthLimit: Move file directly under resources/src/

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

Change 542640 merged by jenkins-bot:
[mediawiki/core@master] jquery.lengthLimit: Move file directly under resources/src/

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

Change 571022 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/WikimediaEvents@master] Strip 'ext.wikimediaEvents' prefixes from file names

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

Change 571022 merged by jenkins-bot:
[mediawiki/extensions/WikimediaEvents@master] Strip 'ext.wikimediaEvents' prefixes from file names

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

Change 579356 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/TemplateData@master] Organise resource files in directories after their module bundles

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

Change 579356 merged by jenkins-bot:
[mediawiki/extensions/TemplateData@master] Organise resource files in directories after their module bundles

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

Change 627914 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/CheckUser@master] resources: Organize frontend files by module directory

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

Change 627914 merged by jenkins-bot:
[mediawiki/extensions/CheckUser@master] resources: Organize frontend files by module directory

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

Change 649463 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] mediawiki.action.edit: Move module files to their own subdirectory

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

Change 649463 merged by jenkins-bot:
[mediawiki/core@master] mediawiki.action.edit: Move module files to their own subdirectory

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

From https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/661116 for wider visibility:

@Krinkle in 'skin.json' Line 135
In the new module-based directory model, we don't usually repeat module names in file names. These could be legacy.less and skin.less, for example. See T193826 for a few dozen examples.

Some open questions:

Module naming conventions should be documented within https://www.mediawiki.org/wiki/ResourceLoader/ Closest point might become https://www.mediawiki.org/wiki/ResourceLoader/Architecture#Modules ?

To clarify, the comment above would result in a folder & file name structure

  • 'skins.vector.styles/skin.less' – 'skin' chosen alongside other similar entry points like 'skin.js'
  • 'skins.vector.styles.legacy/skin-legacy.less' – This should actually be 'legacy.less', but due to it being an entry point a compromise similar to 'skin-legacy.js' seems fine…?!

Looking at other structures:

  • 'mediawiki.diff.styles' features 'diff.less', so the 'styles' folder name suffix that signifies style-only modules is always ignored and the last part of the module name is selected for the filename, here 'diff'.
  • 'mediawiki.htmlform.styles' features 'styles.less', but has an 'images' folder. This should result in two decisions: 1. It's fine for styles modules to have 'images' folders, 2. The filename should be changed to 'htmlform.less'
  • 'mediawiki.icon' – no .styles – features 'icon.less', has an 'images' folder. See above for acceptance of 'images' folder. '.styles' should be added to the folder name.
  • 'mediawiki.special.preferences.ooui' folder incl JS, while 'mediawiki.special.preferences.styles.ooui.less' instead of 'mediawiki.special.preferences.ooui.styles.less'? Latter seems correct in the grand naming scheme.

From task description:

  • "mediawiki.widgets.datetime" is at resources/src/mediawiki.widgets.datetime/. This should be renamed to 'datetime.js' and 'datetime.less' accordingly?!

See https://www.mediawiki.org/wiki/Manual:Coding_conventions#File_naming.

I don't think there should be rules about local file names like that. Just pick something short that makes sense to you.

I agree that for the "entry point" file there probably isn't much need or room for creativity. I typically pick either index.js or <last significant part>.js. But assuming you have more than one file in that directory, the other files need names as well. Whatever logic you use for those you can use for the entry point as well. I don't think we loose any productivitity or comprehension speed/value from picking these in a way that is not perfectly formalised and predictable.

Having said that, this ticket was mainly motivated by lack of discoverability and confusion around where module boundaries are and how to quickly find them in a text editor. That is addressed with directories. For files I haven't really thought about it very much. If you have specific ergonomic issues there that you'd like to see address, I think we could adopt a convention there if that would help. Open to ideas there and to hear what others thing. For myself, I haven't yet felt ergonomic issues there, nor do I have any stylistic preference for it, other than to not repeat/duplicate the full module name.

Change 692980 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/extensions/UploadWizard@master] ext.uploadWizard.uploadCampaign: Merge module with just 1 line of CSS

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

Change 692980 merged by jenkins-bot:

[mediawiki/extensions/UploadWizard@master] ext.uploadWizard.uploadCampaign: Merge module with just 1 line of CSS

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

Change 738026 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] mediawiki.ui: Organize files by module bundle

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

Change 738026 merged by jenkins-bot:

[mediawiki/core@master] mediawiki.ui: Organize files by module bundle

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

Change 763746 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] mediawiki.action.styles: Move file out of mediawiki.action/ folder

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

Change 763746 merged by jenkins-bot:

[mediawiki/core@master] mediawiki.action.styles: Move file out of mediawiki.action/ folder

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

Change 813355 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] tests: Fix outdated /tests/qunit structure and flatten a bit

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

Change 813355 merged by jenkins-bot:

[mediawiki/core@master] tests: Fix outdated /tests/qunit structure and flatten a bit

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

Change 815614 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] tests: Flatten /tests/qunit for jquery.* module tests as well

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

Change 815614 merged by jenkins-bot:

[mediawiki/core@master] tests: Flatten /tests/qunit for jquery.* module tests as well

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

Change 957829 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] mediawiki.page.watch.ajax: Move into separate dir to improve source map

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

Change 957831 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] mediawiki.langauge: Move related modules and virtual files to separate dir

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

Change 957829 merged by jenkins-bot:

[mediawiki/core@master] mediawiki.page.watch.ajax: Move into separate dir to improve source map

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

Change 957831 merged by jenkins-bot:

[mediawiki/core@master] mediawiki.language: Move related modules and virtual files to separate dir

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