Proptypes; more pain than gain?

[peterbe]

Having to maintain the proptypes is a pain. It's like bureaucratic paperwork that you have to do because a computer can't do it for you. And the warnings are clearly not doing us much good because we constantly fail the proptypes and those failings are left in committed code and don't break the CI on pull requests.

Three options:

1. Ditch it entirely
2. Make prop warnings something can cause an error instead, to our attention sooner (especially in CI)
3. Keep it as is

I know I'm not the lead on this project and I have yet to see any benefit but perhaps others feel it really useful, in that case, vote for option 3.

[magopian]

We currently have no other way than using PropTypes to have some level of trust in what type we're expecting/sending/using.

Maintaining flow was dropped because it was perceived as too much work versus the gain. It seems we're feeling somewhat the same about PropTypes?

I'm not sure what to think of that issue: I would feel more confident dropping PropTypes (and flow also, in retrospect) if we had better test coverage (and ways to track this coverage).

I have the (maybe wrong) feeling that this is a big issue we're facing regarding the maintainability of the project: how do we make sure that interfaces are working correctly? Do we use types? Do we use unit testing? Do we use integration testing? What's the strategy?

We could also obviously simply decide that we don't need types or more tests, given that: 1/ the console isn't "user facing", it's "only" used by a handful of users inside Mozilla 2/ important bugs (ones that prevent using the console at all) will be caught early, and we'll be able to answer/fix quickly, or rollback 3/ the current size and complexity of the console is still manageable (there's only the Normandy frontend for now)

[mythmon]

I don't think that simply dropping the idea of test and type coverage is a good idea. Even at its current size, we've already run into several problems that a well typed system would have caught (both in DC and in the former Normandy Admin incarnation). Our users may be "only" a handful of users inside Mozilla, and much better than the average user at reporting bugs, but the audience is only going to grow, and they have a job to do that isn't testing DC.

I wasn't involved while Flow was being used, and I don't know the problems it had. I can say that I've been having good success with Typescript on a create-react-app based react/redux side project that uses a similar architecture to DC. The tooling support (in VSCode) has been really great, and besides some hang up with @connect it has been going well (in short, connect can't be well typed and also a decorator in Typescript). This kind of typing is rather noisy though, and adds a lot more code to read. I'm personally happy with the tradeoff, but not everyone is.

If proptypes could fail the build, or at least the tests, I would be on board with them. As it is, they don't cause CI to fail, and so they are always second class failures, like deprecation warnings we will ignore until they cause a bug. Given that the state of the art in detecting proptype failures is, as far as I can tell, mocking console.error, I'm not very optimistic on this front.

[rehandalal]

they are always second class failures, like deprecation warnings we will ignore until they cause a bug.

Ostensibly with the new Sentry set up these warnings would be surfaced and if we connect Sentry to GH issues we would have issues for every PropType warning.

This is far from ideal (and certainly having the build fail is a much more reliable way to ensure these issues don't become bugs for users) but it means these should hopefully not go completely ignored.

[magopian]

I'm totally on board with the premise that having a type system and immutable data types raises both the quality of the project, and the maintainability. If I chose elm as my favorite language (whenever I'm allowed to use it) is for a good reason: it makes the development a breeze (kind of like TDD), and allows me to get back to a project months later and have no mental overhead to get back to developing it straight away.

That being said, we're talking here about bolting an incomplete type system with bad error messages and "type black holes" on a dynamic language, and immutable data types on a very mutable language.

I wasn't involved while Flow was being used, and I don't know the problems it had.

I'm not sure the decision to remove flow was good, but what I'm sure is that I'm very aware of the limitations:

1/ the type systems for javascript are incomplete: what that means is that not everything has to be typed, and you may unknowingly end up with type black holes (the whole article is an interesting read), which voids a lot of the advantages of having static types in the first place 2/ this first point means you NEED the tooling to maybe tell you where those holes are, using something like flow-coverage-report, and come up with some way to fail the builds whenever the coverage goes below some threshold (which threshold should that be is a whole other can of worms), as I believe flow-coverage-report is still not supported by tools like coveralls 3/ according to this research the gain is roughly 10 to 15% of bugs that flow or typescript will catch, which has to be balanced with the overhead of

• adding and maintaining the types (which is way more complicated and frustrating when you don't start a project with flow but add it later on, like we did on the delivery-console)
• monitoring and maintaining the tooling

My conclusion is "to avoid having to make tradeoffs, we should change the language, which is the root cause of our problems". As that's not a possibility, we're left with having to decide, without much data, how much we're willing to invest in time/tooling/maintenance to slightly increase the quality and the maintainability of the project.

I know that at least two other teams at Mozilla are using flow

• addons-frontend (no immutable-js, no flow-coverage-report)
• perf-html profiler (with flow-coverage-report, but no immutable-js)

We're also using flow on the kinto-admin( but no immutable-js, and no flow-coverage-report).

[peterbe]

I think what might pivot this discussion towards a resolution is that we install and depend on https://www.npmjs.com/package/jest-prop-type-error. Then proptypes are not allowed to be wrong as the tests will fail.

[rehandalal]

+1

[peterbe]

Let's redirect our energy to https://github.com/mozilla/delivery-console/issues/431