gcanti / fp-ts

Functional programming in TypeScript
https://gcanti.github.io/fp-ts/
MIT License
10.85k stars 503 forks source link

Consider dropping support for mutable modules #1226

Open gcanti opened 4 years ago

gcanti commented 4 years ago

Namely:

SRachamim commented 4 years ago

As a functional library it's OK to enforce immutability.

I would get rid of all Readonly* modules and make the rest immutable.

mlegenhausen commented 4 years ago

Would love to see that readonly is the default in the whole fp-ts ecosystem but I already had my struggle to switch to the readonly instances with fp-ts@2 because fp-ts-contrib, io-ts-types and monocle-ts has not full readonly support. If we could add the readonly instances e. g. ReadonlyZipper for fp-ts-contrib, readonlyNonEmptyArray for io-ts-types and indexReadonlyNonEmptyArray for monocle-ts I would already give it a try.

gcanti commented 4 years ago

and indexReadonlyNonEmptyArray for monocle-ts

@mlegenhausen https://github.com/gcanti/monocle-ts/pull/124

gcanti commented 4 years ago

I would get rid of all Readonly* modules and make the rest immutable

There are two options I guess:

Option 1

Keep the Readonly* modules and simply remove the mutable counterparts (:+1: if you prefer this option)

Option 2

Remove all the mutable modules and rename Readonly<X> module to <X> (:-1: if you prefer this option)

gkamperis commented 4 years ago

@gcanti based on a similar conversation...

this explains my vote :)

SRachamim commented 4 years ago

We can see this issue in two lights:

  1. We're dealing with a strongly-typed pure functional library. We're expecting only readonly data-structures and we're promising to give back only readonly data structures. In this case, the immutability is an axiom, and we don't need to explicitly prefix all our data-structures with a Readonly prefix. We'll have one Array module which is readonly.

...or:

  1. We're a TypeScript library, in which the default is mutable structures. There is already a convention of prefixing with Readonly all immutable data-structures. This is aligned with the current situation, in which we have both mutable Array and immutable ReadonlyArray.

I tend to like the first philosophy.

anilanar commented 3 years ago

Readonly arrays/records (using ts readonly keyword) have low adoption in general even in immutable code bases. If mutable modules are replaced with the readonly variants, our codebase at Userlike would not work because we cannot realistically make all our arrays Readonly even though all of them are immutable.

EricCrosson commented 3 years ago

Since this may hinder adoption by less pure codebases, throwing another option out there: switch the defaults, so the module Array supports only readonly Arrays but also provide e.g. MutableArray

kalda341 commented 2 years ago

I'm currently in a world of hurt working heavily with fp-ts trees and otherwise readonly data, having to constantly be converting between readonly and mutable arrays. Additionally, monocle-ts's index function only supports readonly, so I have lots of nasty casts when working with trees.

This could be solved by moving to all mutable data-structures, or all immutable. Personally I like immutable everywhere. If my codebase is anything to go by, nearly all of my data processing is written in fp-ts. No conversion is required to pass data in (mutable is a superset of immutable), and it's trivial in most cases to perform a call such as toMutableArray and the end of processing when it's required. In most cases where it's "required" by Typescript, it isn't even necessary, it's just the developer of a library didn't add readonly to their input types. I use the following Typescript trick in those cases:

export type DeepWritable<T> = { -readonly [P in keyof T]: DeepWritable<T[P]> };

export function castWritable<T>(x: T): DeepWritable<T> {
  return x as DeepWritable<T>;
}
ammut commented 1 year ago

I have been stumbling over traverseArray: (f: (a: A) => F<B>) => (as: readonly Array<A>) => F<READONLY Array<B>> so many times. In my opinion, it's perfectly fine and admirable to type your library functions so their parameters are readonly - in essence you are making a promise to whomever uses your library, that this function won't alter its arguments.

However, if you type your functions so their return values are readonly you are in essence saying: YOU may not alter this array, and there is a good reason for it. Which in the case of traverseArray just plainly isn't true. It's just an array, it has passed over into my hands and my responsibility, why do I need to recast before I may .push()?