Page MenuHomePhabricator

Maplink implementation throws a JS TypeError
Closed, ResolvedPublic

Description

When clicking on a maplink at https://ro.wikipedia.org/w/index.php?title=Lista_stațiilor_de_metrou_din_București in the latest version on Chrome on ChromeOS, the map does not load and the console shows an error:

Uncaught TypeError: externals[key].fetch is not a function
    at InternalGroup.Group_Hybrid.HybridGroup.fetchExternalGroups (wikimedia-mapdata.js?e8676:408)
    at Object.<anonymous> (wikimedia-mapdata.js?e8676:491)
    at Object.<anonymous> (load.php?debug=true&lang=ro&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=1u1uxh5:3305)
    at fire (load.php?debug=true&lang=ro&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=1u1uxh5:3148)
    at Object.add [as done] (load.php?debug=true&lang=ro&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=1u1uxh5:3194)
    at Array.<anonymous> (load.php?debug=true&lang=ro&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=1u1uxh5:3304)
    at Function.each (load.php?debug=true&lang=ro&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=1u1uxh5:384)
    at Object.<anonymous> (load.php?debug=true&lang=ro&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=1u1uxh5:3301)
    at Function.Deferred (load.php?debug=true&lang=ro&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=1u1uxh5:3361)
    at Object.then (load.php?debug=true&lang=ro&modules=jquery%2Cmediawiki&only=scripts&skin=vector&version=1u1uxh5:3300)

Event Timeline

Strainu created this task.Dec 25 2016, 9:05 PM
Restricted Application added a project: Discovery. · View Herald TranscriptDec 25 2016, 9:05 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Strainu edited projects, added Maps (Kartographer); removed Maps.Dec 25 2016, 9:06 PM

This seems specific to ro.wp. I took the example from [[:mw:Help:Extension:Kartographer]] and tested with it on both mw.org (worked) and ro.wp (did not work).

<maplink zoom=9 latitude=37.8013 longitude=-122.3988>
{
  "type": "Feature",
  "geometry": { "type": "Point", "coordinates": [-122.3988, 37.8013] },
  "properties": {
    "title": "Exploratorium",
    "marker-color": "228b22",
    "marker-symbol": "-number-see"
  }
}
</maplink>
Strainu updated the task description. (Show Details)Dec 25 2016, 9:33 PM
Yurik added a subscriber: Yurik.

Thanks, will investigate shortly!

Further investigation showed that this is caused by our version of Twinkle. I am leaving this bug open in the hope that I can get some help on how they interact causing this. The backtrace does not seem to involve Twinkle at all, so I must be missing something. I am hoping that the Kartographer team can indicate what exactly goes wrong in their code so I can use that information to debug Twinkle. Thanks!

@Strainu, @Yurik is it still reproducible?

I clicked on a few links, such as https://ro.wikipedia.org/w/index.php?title=Lista_sta%C8%9Biilor_de_metrou_din_Bucure%C8%99ti#/maplink/5, and it doesn't trigger any error.

@JGirault , I mentioned above that this is linked to our version of Twinkle (you need to enable the gadget to reproduce) and that I would appreciate any suggestion on what is exactly going wrong - I just can't understand how can Twinkle affect that particular code - there is nothing called "externals" or "fetch" in either of the Twinkle scripts. I also tried looking at the up-to-date TW source to see if I could find something to fix this.

@Strainu TLDR; I identified the bug, the solution is easy, I'm gonna make a patch right after.

There are several components of the problem:

Array.prototype.uniq = function arrayPrototypeUniq() {
	var result = [];
	for( var i = 0; i < this.length; ++i ) {
		var current = this[i];
		if( result.indexOf( current ) === -1 ) {
			result.push( current );
		}
	}
	return result;
};

It extends the base object like this with 3 methods: uniq, dups, chunk. Extending the base object is a bad practice in general, as it can lead to issues that are very hard to troubleshoot.

  • Our Kartographer script that downloads the data makes a bad use of the for ... in loop, when iterating over an array instead of an object. Since the base Array object is extended, any array would have the properties uniq, dups and chunk. An easy way to see that there is a problem is to type in the console:
for (var key in []) {
    console.log( key );
}

returns:

uniq
dups
chunk

Solution:
I'm gonna change the for ... in to a regular for loop, and it will fix it. No need to change w:ro:MediaWiki:Gadget-morebits.js, although it is still a bad practice to extend the base object.

JGirault claimed this task.Jan 3 2017, 9:48 PM
JGirault moved this task from Backlog to In progress on the Maps-Sprint board.

Change 330316 had a related patch set uploaded (by JGirault):
Fix the for...in loop that should be a for loop

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

Change 330316 merged by jenkins-bot:
Fix the for...in loop that should be a for loop

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

Change 330320 had a related patch set uploaded (by JGirault):
Upgrade mapdata lib to fix a bug on rowiki

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

JGirault moved this task from In progress to Done on the Maps-Sprint board.Jan 3 2017, 10:26 PM

Change 330320 merged by jenkins-bot:
Upgrade mapdata lib to fix a bug on rowiki

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

Change 330321 had a related patch set uploaded (by Yurik):
Upgrade mapdata lib to fix a bug on rowiki

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

Change 330322 had a related patch set uploaded (by Yurik):
Upgrade mapdata lib to fix a bug on rowiki

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

Change 330321 merged by jenkins-bot:
Upgrade mapdata lib to fix a bug on rowiki

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

Change 330322 merged by jenkins-bot:
Upgrade mapdata lib to fix a bug on rowiki

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

Thank you, that great news, it means that we'll be able to upgrade {{coord}} to maplink soon.

Verified, I think this can be closed now.

debt closed this task as Resolved.Jan 17 2017, 5:15 PM
debt added a subscriber: debt.

Thanks, @Strainu - resolving!