Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider dropping support for mutable modules #1226

Open
gcanti opened this issue Jun 3, 2020 · 10 comments
Open

Consider dropping support for mutable modules #1226

gcanti opened this issue Jun 3, 2020 · 10 comments
Milestone

Comments

@gcanti
Copy link
Owner

gcanti commented Jun 3, 2020

Namely:

  • Array
  • Map
  • NonEmptyArray
  • Record
  • Set
  • Tuple
@gcanti gcanti added this to the v3 milestone Jun 3, 2020
@SRachamim
Copy link
Contributor

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

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

@mlegenhausen
Copy link
Collaborator

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
Copy link
Owner Author

gcanti commented Jun 4, 2020

and indexReadonlyNonEmptyArray for monocle-ts

@mlegenhausen gcanti/monocle-ts#124

@gcanti
Copy link
Owner Author

gcanti commented Jun 7, 2020

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
Copy link

@gcanti based on a similar conversation...

this explains my vote :)

@SRachamim
Copy link
Contributor

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.

@gcanti gcanti mentioned this issue Jul 22, 2020
4 tasks
@anilanar
Copy link
Contributor

anilanar commented Jan 15, 2021

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
Copy link
Contributor

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
Copy link

kalda341 commented Jun 7, 2022

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
Copy link

ammut commented Oct 12, 2023

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()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants