Page MenuHomePhabricator

resource-modules linter fails because mw.track is defined twice
Closed, ResolvedPublic

Description

MobileFrontend makes use of a linter that parses and runs JavaScript to determine where things are defined and that dependencies and messages expressed inside extension.json are correct.
It's currently living here: https://github.com/joakin/resource-modules

When the job fails code inside MobileFrontend is unmergeable.

This job is currently failing after a change to mw.track inside https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/437914/ which seems to define mw.track twice which confuses the parser.

Example of a failing job:
https://integration.wikimedia.org/ci/job/mwext-MobileFrontend-npm-run-lint-modules-docker/752/console

21:13:50 Error: Dependency problems in file: resources/mobile.search/MobileWebSearchLogger.js:
21:13:50   Required mw.track defined in multiple files:
21:13:50     resources/src/mediawiki/mediawiki.base.js
21:13:50     resources/src/mediawiki/mediawiki.js
21:13:50

I've asked for clarification from performance team, but we may need to update resource-modules again. :(

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 14 2018, 9:54 PM
Krinkle added a subscriber: Krinkle.EditedJun 14 2018, 10:26 PM
  • @Jdlrobson Doesn't this mean mw.track is now defined twice? Inside mediawiki.base and then mediawiki?
  • @Krinkle Yes, that is correct and is working as intended.

The second definition in mediawiki.base.js wraps the original by extending it.

As for the "resource-modules" linter, this is the first I've heard of it. I'm not sure how it works internally, but it might be easier to give it a stub data for core interfaces rather then indexing them in this way. The public APIs are stable, documented and thoroughly tested. If breaking changes happen in the future, those will follow the deprecation policy. Stubbing it would mean you could maintain only a map of module names to public methods, changing only when new methods are used, or after their removal. Not being subject to internal file structure and content.

We are changing the behavior of the linter to not complain about multiple definitions on global variables, and instead check that at least one of the files with the definitions are in the dependency tree.

That way we avoid this errors and get some potential checks for the future.

Ticket to be updated when we publish the fix.

resource-modules@2.0.0 published which should fix this issue. MobileFrontend patch needed to update the version of the lib.

Change 440551 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Fix: upgrade resource-modules linter

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

Change 440551 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Fix: upgrade resource-modules linter

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

Niedzielski added a subscriber: Niedzielski.

@Jdlrobson over to you for sing off.

..It might be easier to give it a stub data for core interfaces rather then indexing them in this way. The public APIs are stable, documented and thoroughly tested. If breaking changes happen in the future, those will follow the deprecation policy. Stubbing it would mean you could maintain only a map of module names to public methods, changing only when new methods are used, or after their removal. Not being subject to internal file structure and content.

@Jdlrobson, should I make a task for this?

Krinkle moved this task from Limbo to Perf issue on the Performance-Team (Radar) board.
Jdlrobson closed this task as Resolved.Jun 19 2018, 9:28 PM

Let's keep an eye on this module. I'd rather deal with issues as they arise.

Change 442087 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Fix: upgrade resource-modules linter

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

Change 442087 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Fix: upgrade resource-modules linter

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

Vvjjkkii renamed this task from resource-modules linter fails because mw.track is defined twice to izaaaaaaaa.Jul 1 2018, 1:03 AM
Vvjjkkii reopened this task as Open.
Vvjjkkii removed Jdlrobson as the assignee of this task.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed subscribers: gerritbot, Aklapper.
CommunityTechBot renamed this task from izaaaaaaaa to resource-modules linter fails because mw.track is defined twice.Jul 2 2018, 7:17 AM
CommunityTechBot closed this task as Resolved.
CommunityTechBot assigned this task to Jdlrobson.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added subscribers: gerritbot, Aklapper.