Page MenuHomePhabricator

Lint JS sources dependency graph against ResourceModules configuration
Closed, DeclinedPublic

Description

Intro

Currently ResourceModules in extension.json is the source of truth of the JS dependency graph and which assets will be served.

At the same time, the JS sources explicitly define dependencies on other modules by using them in the source or requireing them, giving form to a parallel dependency graph specified by the source code.

Such source-defined dependencies may get out of sync or be mis-configured with the separate ResourceModules configuration, giving place to ResourceLoader serving assets with order/dependencies that are not really the constraints defined by the source files.

This results in non-deterministic JS exceptions in the runtime, missing messages at runtime, bad dependency order in ResourceModules configuration, etc.

Some examples: T146031 T145566 T99461

What

We want a linter that parses the frontend sources of extensions and builds a dependency graph from the source, and then checks that those constraints specified by the sources are reflected in extension.json ResourceModules.

That way, extension.json ResourceModules will for sure reflect the needs of the source code. We want that linter to run on CI so that no sources on master are out of sync with extension.json config.

Things to be linted (current = current js file, current-RLmodules = ResourceModules that include current in scripts):

  • Messages in current are in current-RLmodules
  • Required modules/function/variables in current are somewhere in the dependencies of current-RLmodules
    • For legacy M.require/M.define
    • For RL new require/module.exports
    • For global JS variables in mw (mw.blah.blah)
  • Templates used in current are in current-RLmodules

How


Please, help review

  • Nothing right now

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Jhernandez moved this task from Incoming to Epics/Goals on the Web-Team-Backlog board.

Change 312986 had a related patch set uploaded (by Hashar):
[MobileFrontend] exp job for npm run lint:modules

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

Change 312986 merged by jenkins-bot:
[MobileFrontend] exp job for npm run lint:modules

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

Change 312850 had a related patch set uploaded (by Jhernandez):
WIP: Add resource modules linter

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

Change 312988 had a related patch set uploaded (by Jhernandez):
Avoid calling mw.msg with variables

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

Change 312991 had a related patch set uploaded (by Jhernandez):
Make Nearby getDistance test diff readable

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

Change 313008 had a related patch set uploaded (by Jhernandez):
Make loadModule use string literals instead of variables

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

Change 313010 had a related patch set uploaded (by Jhernandez):
Remove dead code CommonsCategoryOverlay

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

Change 313011 had a related patch set uploaded (by Jhernandez):
Add messages used in sources to the modules where they are used

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

Change 313012 had a related patch set uploaded (by Jhernandez):
Import mw.ForeignApi to local scope instead of directly using it

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

Change 313015 had a related patch set uploaded (by Jhernandez):
Move messages in config to where they are use in the source

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

Change 313009 had a related patch set uploaded (by Jhernandez):
Move mobile.browser/browser to mobile.browser/Browser#getSingleton

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

Change 312991 merged by jenkins-bot:
Make Nearby getDistance test diff readable

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

Change 312988 merged by jenkins-bot:
Avoid calling mw.msg with variables

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

Change 313008 merged by jenkins-bot:
Make loadModule use string literals instead of variables

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

Change 313009 merged by jenkins-bot:
Move mobile.browser/browser to mobile.browser/Browser#getSingleton

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

Change 313010 merged by jenkins-bot:
Remove dead code CommonsCategoryOverlay

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

Change 313015 merged by jenkins-bot:
Move messages in config to where they are use in the source

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

Change 313012 merged by jenkins-bot:
Import mw.ForeignApi to local scope instead of directly using it

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

Change 313205 had a related patch set uploaded (by Jhernandez):
Move mobile-frontend-editor-anon to where it is used

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

Change 313011 merged by jenkins-bot:
Add messages used in sources to the modules where they are used

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

Change 313205 merged by jenkins-bot:
Move mobile-frontend-editor-anon to where it is used

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

Change 313980 had a related patch set uploaded (by Jhernandez):
Make mobile.drawers depend on mobile-frontend-overlay-close

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

Change 313980 merged by jenkins-bot:
Make mobile.drawers depend on mobile-frontend-overlay-close

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

This work is great. Thanks for pushing it, everyone who is involved. @Jhernandez let me know how I can help with getting this running for all of our patches in all extensions that we maintain.

Jdlrobson changed the task status from Open to Stalled.Nov 16 2016, 5:05 PM
Jdlrobson subscribed.
Jhernandez changed the task status from Stalled to Open.Mar 6 2017, 12:47 PM

I'm back to working on this. Seems pretty stable now, see https://gerrit.wikimedia.org/r/#/c/312850/18

I sent an email with a question about a warning in MobileFrontend, I'll come back with the answer.

We've chatted and the next step is

That way we can disable parsing of parts of the code and avoid error reporting in special situations like in [MobileFrontend/resources/mobile.editor.overlay/EditorOverlay.js#L165](https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/master/resources/mobile.editor.overlay/EditorOverlay.js#L165) where 'visualeditor-mweditmode-tooltip' is used but not defined in module config because it is defined in another extension.

^ Since Joaquin said he might want to work on this next sprint let's identify what that work is and attempt to schedule it.

Jhernandez updated the task description. (Show Details)
Jhernandez updated the task description. (Show Details)

eb201e4: Add resource modules linter is ready for review.

Last release of the linter: v1.0.15...v1.0.17 (adds disabling via comment syntax)

All linting problems have been fixed on MobileFrontend, right now. @Jdlrobson I'd appreciate some review.


After merging this the followup for the task is talking to Antoine about making it a stable job for MobileFrontend, and maybe seeing remaining issues in other extensions like QuickSurveys, RelatedArticles, etc.

Change 347051 had a related patch set uploaded (by Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Lint modules as part of npm-node-6-jessie

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

Change 312850 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Add resource modules linter

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

Change 347051 abandoned by Jdlrobson:
Lint modules as part of npm-node-6-jessie

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

@Jdlrobson Re: ^ linting on the npm job: The reason why it is a special job and not an npm one is because for linting we need the whole source tree (core and extension) and in the npm job only the extension is cloned :(

^ Since Joaquin said he might want to work on this next sprint let's identify what that work is and attempt to schedule it.

Was this effective? In my head this remains a 10% project and these tasks are more for capturing thoughts/representing the size of this (awesome) undertaking.

I'll move this back to Epics in the meantime as T160056: Add disabling comment syntax to resource-modules linter, for example, is in Sprint +1.

We got half of that task done so I'd say yes.
It's a 10% project but it's something the group has identified as being useful. Interest in 10% projects fizzles out naturally and I see it as our duty as a team to help see good ideas come to full fruition. I want to at least wrap up this work for MobileFrontend so we can evaluate it's usefulness.

Jdlrobson raised the priority of this task from Medium to High.Apr 26 2017, 3:21 PM

To reflect the fact we are close to finishing this.

@Jhernandez thanks for all your work here! :) Would it make sense to do this for other extensions too?

Well done!! From a quick chat I had with @Jdlrobson , it would be rather nice to have that RL linter to be generalized so it can benefits other extensions. Maybe the linter could be upstreamed to mediawiki/core ?

That would be very interesting. As next steps, as seen on the description, I want to run it on more extensions and fix issues that come up. I'll see what I can tackle next.

Jdlrobson lowered the priority of this task from High to Low.Jun 2 2017, 8:23 PM

I think now we are using Webpack, this is not the direction we are taking.