Make all component inputs immutable about peoplemath HOT 3 CLOSED

Using datatypes which ensure runtime immutability may result in a lot of access syntax needing to change.

An alternative would be to use Typescript features like ReadOnly, combined with a technique such as the Immutable interface discussed here, to enforce immutability at compile time only.

from peoplemath.

I tried to implement compile-time immutability enforcement by using an Immutable recursive type (as discussed above) to create equivalents of the existing model types with all properties readonly. I converted many components to use such types for their inputs, e.g. Immutable<Period> instead of Period. However, I ran into some problems.

I'm looking for as much type safety as possible in the solution here: we want any attempt to modify immutable data structures to result in a compile error. If any such mutations still occur on any code path, switching to OnPush will result in very nasty bugs, because Angular's change detection will not see those mutations and the UI will fail to update.

Unfortunately, TypeScript's enforcement of readonly leaves a lot to be desired. In particular, I was very surprised and disappointed to discover microsoft/TypeScript#13347, which means that if type T has no properties of array or tuple types, values of type Immutable<T> are still assignable to T. This is a serious problem, because it creates a gaping hole in the type system: for example, if there is some function somewhere that wants a T, when we convert a component's @Inputs to Immutable<T>, the compiler will not catch cases where we are passing the input to that function, which is then free to make mutations of precisely the sort we are trying to prevent.

One of the commenters on that issue pointed out an ESLint rule that is supposed to provide the missing type safety, but Angular is not using ESLint yet (angular/angular-cli#13732) and Angular ESLint tooling such as this seems to be in a very immature state of development still. I had a go at switching to ESLint for PeopleMath, but I ran into some obscure errors. I'm sure this could be made to work eventually, but I don't like deviating from Angular's standard toolchain too much, and I think it's best not to make this change now.

Even if we could enable this, enforcement of readonly doesn't seem to be something TypeScript takes very seriously at the moment, and until that changes, I don't think we can get enough compile-time immutability guarantees to be comfortable abandoning runtime enforcement.

That rules out this Immutable-based solution, as well as libraries such as Immer, unless the auto-freeze mode is used in production, which is not recommended.

I think I'll try using Immutable, which does provide runtime guarantees of immutability. We can maintain type safety by writing wrapper classes for each model type T which implement Immutable<T>. In the cases where the compiler can't prevent mutation, we should at least then get a clear runtime error.

Given the apparently unmaintained state of Immutable (immutable-js/immutable-js#1689), this isn't ideal, but the only alternative I see is to write a parallel set of immutable model types, which would also have its problems (e.g. how to make sure ImmutablePeriod isn't assignable to Period).

from peoplemath.

This is fixed by the merging of pull request #43, with some very satisfactory performance improvements! See the pull request for specific benchmark details.

In the end, I did not use Immutable, and wrote my own immutable types instead. This seems to work pretty well, and it has some nice advantages, e.g. quite a bit of business logic can be moved into those classes, which makes it much easier to test.

from peoplemath.