openwpm / OpenWPM

A web privacy measurement framework
https://openwpm.readthedocs.io
Other
1.33k stars 314 forks source link

Remove TypeScript usage from repo (and Webpack?) #529

Open nhnt11 opened 4 years ago

nhnt11 commented 4 years ago

I wanted to start the conversation about this. Here are some pros:

  1. Removal of a dependency and a compile-step.
  2. Increases the ease of contributing (I'd argue that TS devs can write JS more easily than JS devs can write TS)
  3. No need to mirror standard Web interfaces as TS types. This is a huge annoyance for me.
  4. The instrumentation is really not that complex and I feel like enforcing a type system is really overkill.
  5. Since this is a Web measurement tool, I argue that using vanilla Web technologies is more sustainable.
  6. Our linting setup is not very polished and leads to errors that are quite difficult to figure out (Steve and I spent probably 30-60 minutes on simple lint issues when trying to hack on stuff last week).

I (and other Firefox devs, I'd wager) prefer vanilla JS. @motin I know you introduced TS to this codebase - what do you think? @englehardt?

I wonder if we should also do away with Webpack, but I haven't thought about it much. Without TS and Webpack, the extension would be way easier/faster to develop and debug IMO.

motin commented 4 years ago

In it's current mode (stable/maintenance) the upside of TS is definitely smaller than during testless Quantum-support-refactoring. I'd also argue that Python contributors prefer JS over TS, and since maintenance and contributor compatability is of highest priority, dropping TS/Webpack seems desirable.

I introduced TS since converting the instrumentation to use Web Extension API:s was a paradigm case of where TS is useful. The enterprise consisted of taking a large amount of different (but sometimes only slightly so) events with many different properties from the Web Extensions API and transforming them into a similar amount of different objects with a similar amount of properties (the OpenWPM schema). TS provided code-completion and fast failure on typos during this development stage, avoiding countless issues that may not have been discovered until live deployment or data analysis (OpenWPM tests covering the webext-based instrumentation were not available at the time).

I am all for using clearly defined contracts to define borders and responsibility between modules, but the instrumentation<->backend contract does not have to be implemented in TS, it can also be implemented as a comprehensive set of (JS/TS/python) tests that catches typos during development and verifies that the output from the instrumentation follows the correct schema.

One thing to remember is that the instrumentation is (or was, if this has changed) supposed to be consumable by other backends than the existing OpenWPM python backend (former examples include https://github.com/mozilla/jestr-pioneer-shield-study and https://github.com/birdsarah/faust-selenium). Having up to date type definitions for other consumers may be useful in those cases, but of course type definitions can also be generated from vanilla JS code with jsdoc annotations, or we can just keep a copy of the OpenWPM schema type definition up to date.

Some notes:

jonathanKingston commented 4 years ago

My understanding is you can do tsdoc without the need for Typescript which means you get the type checking in standard JavaScript.

gunesacar commented 4 years ago

This is also a significant barrier for researchers who want to use OpenWPM, but need some simple modifications.

vringar commented 4 years ago

@gunesacar I'm sorry, I don't quite understand which side you are arguing for at the moment? Do you think types make it easier to work with the code base or that this rigidness is harming quick experimentation?

gunesacar commented 4 years ago

Oh, sorry. The latter; the rigidity and (more importantly) the unfamiliar language (TypeScript) harming quick experimentation.

englehardt commented 4 years ago

I'm in favor of removal as well. Particularly hearing the concerns from Gunes as they validate a concern I've had--that TS will be difficult for folks to quickly engage with the repo.

birdsarah commented 4 years ago

My background is in being a huge proponent of TypeScript. I spent many years as a full-stack web developer and, although I most strongly identify with being a python developer, I have developed multiple large applications in JS (of note in terms of number of users and application scale - Bokeh and the Anaconda Enterprise Platform). In that context, moving to TypeScript was one of the greatest things that ever happened to me as a JS developer. @motin explained the core reason why extremely articulately:

I introduced TS since converting the instrumentation to use Web Extension API:s was a paradigm case of where TS is useful. The enterprise consisted of taking a large amount of different (but sometimes only slightly so) events with many different properties from the Web Extensions API and transforming them into a similar amount of different objects with a similar amount of properties (the OpenWPM schema). TS provided code-completion and fast failure on typos during this development stage, avoiding countless issues that may not have been discovered until live deployment or data analysis (OpenWPM tests covering the webext-based instrumentation were not available at the time).

For me this manifested, in particular, in the ability to undertake large refactorings with speed, without breaking things in a code base with only minimal test coverage.

The trade-off that seems to me to be: @gunesacar's desire to be able to quickly hack at things vs. the benefits of TS to help facilitate maintenance of a codebase with a greater safety net in place.

Now, safety nets come in lots of shapes and sizes - and great test coverage also satisfies my concerns. I have not worked on a JS codebase with great test coverage (compared to python codebases) - that is not to say its not possible. Coming from a python background I found JS testing frameworks tricky, and clumsy and, for sure, I enjoyed using TS to be able to avoid writing tests....which is not a great reason.

I'm claiming the subjective benefits that I experienced using TS, so I must acknowledge the subjective pains others are feeling.

Moreover, I do not have enough experience to be able to forcefully claim the benefits of TS in the case of OpenWPM - that is maintainability of a small-medium sized codebase.

So I do not intend to try and override the majority will here, I just wanted to share my experiences.

The secondary reason I love TS was that of writing multi-platform compatible code. While that's arguably not a concern here. I, personally, would like to see OpenWPM support chrome. I do not have an up-to-date opinion on whether these frustrations of the past have eased.

birdsarah commented 4 years ago

I like the suggestions regarding linting and webpack that have been offered to reduce pain. I do not have firm opinions on any particular suggestion for improvement vs the suggestion of removing all together. Others are far more deeply in the weeds than I am.

My only suggestion was to use DefinitelyTyped typings. But, as @vringar pointed out to me this morning, it appears we already are ("@types/firefox-webext-browser": "^63.0.0"). We should probably update this to the most recent which is available 70.0.1. However, as this set of type defs is not maintained by a mozilla person we can expect them to be out of date. https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/firefox-webext-browser/index.d.ts.

And, aside from understanding that there isn't the bandwidth to contribute to upstream projects, I get the sense that resolving the type defs issue would only resolve a subset of frustrations people are experiencing.

vringar commented 3 years ago

Coming back to this I have a couple of thoughts: Our target audience aren't developers/software engineers but researchers, this means the barriers both to modifying existing code and adding new code should be as low as possible. To achieve this result I think having the Instruments be written in TypeScript is valuable, since it allows people looking at the code and know what to expect without having to have MDN open in browser window. What I see in this thread are two major pain points:

  1. The TypeScript definitions for the WebAPIs are out of date meaning we have to write write our own type definitions.
  2. The machinery around building the WebExtension enforces rigorous standards and is quite complex

The first pain point could be a non-issue in the future seeing as the maintainer just updated the API descriptions to FF82 and seems to engage when people interact with them.

The second pain point requires a more wholistic approach.

My suggestions would be:

vringar commented 2 years ago

Looking back at this from a simplifying lense for long-term maintainability: