Page MenuHomePhabricator

[Spike, 4hrs] Explore adding protection for using forbidden methods
Closed, ResolvedPublic0 Story Points

Description

We've already had a blip where we broke Popups for Internet Explorer because we used the const key word without a transpile step in our code (T174424). We easily fixed that and added some safeguard (an additional ESLint job on the distribution script) to stop that happening again.

As we adopt more modern JavaScript via transpiling we'll want to be additionally cautious that we don't inadvertently use unavailable JavaScript in our targetted environment eg. Object.assign

https://github.com/amilajack/eslint-plugin-compat

Spike outcomes

  • A proof of concept demonstrating the feasibility of the idea.
  • A recommendation of whether we go ahead with doing it

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 19 2018, 11:02 PM
Jdlrobson triaged this task as Normal priority.Mar 27 2018, 4:29 PM
Jdlrobson set the point value for this task to 0.Mar 27 2018, 4:48 PM
Niedzielski added a subscriber: Niedzielski.

Tagging popups since it seems like the best project to make this experimentation on.

Change 441090 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/Popups@master] POC: forbid unsupported APIs

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

Change 441090 abandoned by Niedzielski:
POC: forbid unsupported APIs

Reason:
Preemptively abandoning this. Feel free to revive if you disagree.

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

Niedzielski removed Niedzielski as the assignee of this task.Jun 19 2018, 7:22 PM

From POC:

Lint unsupported web API usage with the Browserslist support matrix.
Detection seems simplistic. For example, defining and using a function
called "fetch" triggers a false positive that must either be disabled at
each reference, claiming that fetch has a global polyfill (shown below
via settings), or renaming the symbol (done in this patch). It also
fails to detect missing APIs like Object.entries(). Because of these
shortcomings, I don't think we're going to get as much value out of it
as we'd hoped. However, I'm happy to give it a try if others disagree.

For some reason, overrides work to suppress some issues flagged in tests
but not others which seems like a bug. This patch point suppresses a
Promise usage as a workaround.

Example of claiming is polyfilled in the ESLint settings:

	"settings": {
		"polyfills": [ "fetch" ]
	}

@pmiazga @Jdrewniak wondering if you have any thoughts on the above assessment?

Jdlrobson closed this task as Resolved.Jun 20 2018, 7:28 PM
Jdlrobson claimed this task.

Thanks for looking into this Stephen! I've documented this here: https://www.mediawiki.org/wiki/Reading/Web/Coding_conventions#Transpiling
(We should also look at updating that page to reflect our more modern code practices)

Bummer! I guess it basically just greps for keywords. I don't think defining functions as "polyfills" is a good approach because we might define a function named 'fetch' in one place, but then we might try using the native fetch function in another place, it won't spot that second error.