Page MenuHomePhabricator

Add type hinting to WikimediaEvents to protect against errors in instrumentation
Closed, ResolvedPublic3 Estimated Story Points

Description

Background

Type hinting via TypeScript has been used in Vector. It doesn't require a build step and has effectively detected errors in code before it was shipped.

We have had various issues relating to instrumentation missing fields that I think TypeScripting would help with.

I (@Jdlrobson) raised a patch https://gerrit.wikimedia.org/r/c/mediawiki/extensions/WikimediaEvents/+/670899 but it didn't get the enthusiastic response I was expecting.

Acceptance Criteria

  • By default, all instruments are not type checked with TypeScript
  • An instrument owner can opt-in to type checking their instrument with TypeScript
  • Instruments are type checked during CI

Required

  • Documentation

Notes

  1. Here are some tsconfig.json files from extensions deployed on WMF servers:

Event Timeline

Change 670899 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/WikimediaEvents@master] Add TypeScript to WikimediaEvents

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

Change 670899 abandoned by Jdlrobson:

[mediawiki/extensions/WikimediaEvents@master] Add TypeScript to WikimediaEvents

Reason:

Focusing elsewhere now not going to rebase this one. Capturing in ticket as I'm a bit confused if the patch was controversial or just needed fine tuning.

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

Change 673134 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/WikimediaEvents@master] build: Add TypeScript linting

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

Change 673134 abandoned by Krinkle:

[mediawiki/extensions/WikimediaEvents@master] build: Add TypeScript linting

Reason:

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

We encountered issues in T346106 which would have been detected by TypeScript type checking.

We encountered issues in T346106 which would have been detected by TypeScript type checking.

Concretely, TypeScript would have failed to typecheck an event with a font: string property and then setting that property to the output of mw.user.clientPrefs.get() (string|false) and so the patch couldn't have been merged.


AIUI the original patch and the idea behind it was drafted for discussion in the FSG ~two years ago but it's not clear if it ever was. Since then, however, TypeScript has been used for typechecking in a number of deployed codebases. Codex, the design system for Wikimedia, is written in TypeScript.

I'm for using TypeScript for typechecking in the WikimediaEvents extension. I think that it's the responsibility of instrumentation owners to opt-in to this on a per-instrument basis.

I'm going to ask #front-end Slack channel whether this was discussed and whether there's some formal position about using TypeScript in a codebase. I don't think that we should be discouraged if it hasn't. If anything, I think that this would make an interesting test case to help guide the discussion.

I'm going to ask #front-end Slack channel whether this was discussed and whether there's some formal position about using TypeScript in a codebase. I don't think that we should be discouraged if it hasn't. If anything, I think that this would make an interesting test case to help guide the discussion.

We discussed this task during the FSG meeting on 2023/11/29. I asked for input, especially good and bad experiences using TypeScript as a type checker, rather than consensus about whether this should be done.

@AnneT reminded us that using TypeScript adds a technical barrier to contribution. I didn't mention it at the time but it's worth restating that I'm proposing that this should be opt-in (perhaps indefinitely). The rest of the discussion was rich with stories of JSDoc/TypeScript gotchas, which we resolved to capture on mw:TypeScript.

phuedx updated the task description. (Show Details)

Change 984126 had a related patch set uploaded (by Santiago Faci; author: Santiago Faci):

[mediawiki/extensions/WikimediaEvents@master] Add type hinting to WikimediaEvents to protect against errors in instrumentation

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

The above change is an approach that seems to work fine. It's entirely based on the previous job that @Jdlrobson did previously on his abandoned patch (https://gerrit.wikimedia.org/r/c/mediawiki/extensions/WikimediaEvents/+/670899). I have just made some minor changes to the original idea to accomplish the acceptance criteria about how it should work by default and to make all this optional for instruments creators.

Just a couple of comments:

  • If an instrument creator wants to run a type checking for a specific instrument, its javascript file/s must be added to the include section in the tsconfig.json file (that way no instrument will be type checked by default and type checking will be opt-in for any instrument creator)
  • For any instrument added to the includesection, CI will run the type checking when running the 'posttest' step included in the package.json file (that runs the tsc command line utility)

I'm not absolutely sure this is what we want/need so suggestions/criticism are welcome!

Change 985060 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/WikimediaEvents@master] Add type hinting to clientError.js

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

Change 984126 merged by jenkins-bot:

[mediawiki/extensions/WikimediaEvents@master] Add type hinting to WikimediaEvents to protect against errors in instrumentation

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

Change 985060 merged by jenkins-bot:

[mediawiki/extensions/WikimediaEvents@master] Add type hinting to clientError.js

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

Change 990037 had a related patch set uploaded (by Phuedx; author: Phuedx):

[mediawiki/extensions/WikimediaEvents@master] Document opt-in type checking

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

phuedx set the point value for this task to 3.Jan 16 2024, 10:47 AM

@Sfaci: As we discussed EOD yesterday, could you review https://gerrit.wikimedia.org/r/990037?

Change 990037 merged by jenkins-bot:

[mediawiki/extensions/WikimediaEvents@master] Document opt-in type checking

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