Page MenuHomePhabricator
Create Task
Maniphest T284781

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

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:

Details

SubjectRepoBranchLines +/-
Document opt-in type checkingmediawiki/extensions/WikimediaEventsmaster+16 -0
Add type hinting to clientError.jsmediawiki/extensions/WikimediaEventsmaster+59 -23
Add type hinting to WikimediaEvents to protect against errors in instrumentationmediawiki/extensions/WikimediaEventsmaster+689 -739
build: Add TypeScript lintingmediawiki/extensions/WikimediaEventsmaster+162 -89
Add TypeScript to WikimediaEventsmediawiki/extensions/WikimediaEventsmaster+76 -10
Customize query in gerrit

Related Objects

Event Timeline

Jdlrobson created this task.Jun 10 2021, 8:31 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 10 2021, 8:31 PM
gerritbot added a comment.Jun 10 2021, 8:31 PM

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

gerritbot added a project: Patch-For-Review.Jun 10 2021, 8:31 PM

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

Maintenance_bot removed a project: Patch-For-Review.Jun 10 2021, 9:10 PM
gerritbot added a comment.Jun 10 2021, 10:24 PM

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

gerritbot added a project: Patch-For-Review.Jun 10 2021, 10:24 PM
Jdlrobson moved this task from Incoming to Triaged but Future on the Web-Team-Backlog board.Jun 11 2021, 4:50 PM
Jdlrobson triaged this task as Medium priority.Jun 11 2021, 5:24 PM
Jdlrobson added a project: Desktop Improvements (Vector 2022).
Jdlrobson moved this task from Triaged but Future to Tracking on the Web-Team-Backlog board.Jun 14 2021, 6:23 PM
Jdlrobson edited projects, added Web-Team-Backlog (Tracking); removed Web-Team-Backlog.
Jdlrobson moved this task from Incoming to Deployment on the Desktop Improvements (Vector 2022) board.Aug 31 2021, 9:56 PM
Jdlrobson removed projects: Web-Team-Backlog (Tracking), Desktop Improvements (Vector 2022).
gerritbot added a comment.Dec 20 2022, 11:33 PM

Change 673134 abandoned by Krinkle:

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

Reason:

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

Maintenance_bot removed a project: Patch-For-Review.Dec 21 2022, 12:30 AM
Jdlrobson added a project: Metrics Platform.Oct 30 2023, 4:30 PM

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

Jdlrobson added a subscriber: KSarabia-WMF.Oct 30 2023, 4:43 PM
VirginiaPoundstone added a project: Experimentation Lab.Oct 30 2023, 4:45 PM
phuedx added a comment.Oct 31 2023, 11:04 AM
In T284781#9292157, @Jdlrobson wrote:

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.

VirginiaPoundstone moved this task from Incoming to Wikistats Backlog on the Experimentation Lab board.Nov 2 2023, 2:11 PM
Jdlrobson awarded a token.Nov 7 2023, 10:34 PM
phuedx moved this task from Backlog to Client improvements on the Metrics Platform board.Nov 15 2023, 5:52 PM
Sgs subscribed.Nov 29 2023, 4:21 PM
phuedx added a subscriber: AnneT.EditedDec 5 2023, 4:07 PM
In T284781#9294444, @phuedx wrote:

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)Dec 5 2023, 4:12 PM
phuedx updated the task description. (Show Details)Dec 5 2023, 4:16 PM
phuedx updated the task description. (Show Details)
phuedx edited projects, added Experimentation Lab (Data Products Sprint 05); removed Experimentation Lab.Dec 6 2023, 9:05 AM
Sfaci claimed this task.Dec 13 2023, 6:19 AM
Sfaci moved this task from Sprint Backlog to In Process on the Experimentation Lab (Data Products Sprint 05) board.
gerritbot added a comment.Dec 19 2023, 9:26 AM

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

gerritbot added a project: Patch-For-Review.Dec 19 2023, 9:26 AM
Sfaci added a comment.EditedDec 19 2023, 12:24 PM

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!

Sfaci moved this task from In Process to Code Review / Tech Input on the Experimentation Lab (Data Products Sprint 05) board.Dec 19 2023, 12:44 PM
gerritbot added a comment.Dec 22 2023, 12:56 AM

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

gerritbot added a comment.Jan 9 2024, 1:21 PM

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

• WDoranWMF moved this task from Code Review / Tech Input to Sign Off on the Experimentation Lab (Data Products Sprint 05) board.Jan 9 2024, 1:22 PM
• WDoranWMF moved this task from Sign Off to Done on the Experimentation Lab (Data Products Sprint 05) board.Jan 10 2024, 5:09 PM
phuedx updated the task description. (Show Details)Jan 10 2024, 5:18 PM
gerritbot added a comment.Jan 11 2024, 10:05 AM

Change 985060 merged by jenkins-bot:

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

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

Maintenance_bot removed a project: Patch-For-Review.Jan 11 2024, 10:30 AM
ReleaseTaggerBot added a project: MW-1.42-notes (1.42.0-wmf.14; 2024-01-16).Jan 11 2024, 11:00 AM
gerritbot added a comment.Jan 12 2024, 10:48 AM

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

gerritbot added a project: Patch-For-Review.Jan 12 2024, 10:48 AM
phuedx edited projects, added Experimentation Lab (Data Products Sprint 07); removed Experimentation Lab (Data Products Sprint 05).Jan 12 2024, 10:50 AM
phuedx updated the task description. (Show Details)
phuedx moved this task from Sprint Backlog to Code Review / Tech Input on the Experimentation Lab (Data Products Sprint 07) board.
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?

Sfaci added a comment.Jan 16 2024, 11:18 AM

Done! +1ed

phuedx moved this task from Code Review / Tech Input to Sign Off on the Experimentation Lab (Data Products Sprint 07) board.Jan 16 2024, 12:13 PM
gerritbot added a comment.Jan 16 2024, 12:16 PM

Change 990037 merged by jenkins-bot:

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

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

Maintenance_bot removed a project: Patch-For-Review.Jan 16 2024, 12:30 PM
ReleaseTaggerBot edited projects, added MW-1.42-notes (1.42.0-wmf.15; 2024-01-23); removed MW-1.42-notes (1.42.0-wmf.14; 2024-01-16).Jan 16 2024, 1:00 PM
phuedx moved this task from Sign Off to Done on the Experimentation Lab (Data Products Sprint 07) board.Jan 16 2024, 2:51 PM
• WDoranWMF closed this task as Resolved.Feb 20 2024, 3:50 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits