Dealing with readonly types returned from immutable array operations

[synthet1c]

I'm having a problem using rambda for ngrx selectors that requires me to explicitly set readonly on immutable values to match the typescript definitions in rambda. It is also out of alignment with Ramda which does not force immutable return values through readonly.

Would you consider removing the readonly return type to keep type definitions as concise and compatible as possible? or do you think it's important to force immutable Arrays and Objects?

If I use Rambda.filter in my selector the return type is readonly Foo[]

const selectRambdaFoos = createSelector(
  selectAllFoos,
  Rambda.filter<Foo>(identity), // becomes `readonly Foo[]`
)
// => Selector<object, readonly Foo[]>

If I use Ramda.filter in my selector the return type is Foo[] (although I can't even get it to work, but the type signature should do this)

const selectRamdaFoos = createSelector(
  selectAllFoos,
  Ramda.filter<Foo>(identity),
)
// => Selector<object, Foo[]>

Where using imperative code wouldn't require the interface to be readonly

const selectImperativeFoos = createSelector(
  selectAllFoos,
  foos => foos.filter(R.identity),
)
// => Selector<object, Foo[]>

I can get around it by changing the type where I consume the selected observable, but is there any better way to deal with it? I would like to use rambda for my new job, but don't think it's a good idea at the start to change all their code to suit rambda, and I'm not sure what will happen if I then pipe that value into something else that I don't control that won't accept readonly interfaces.

@Component({ ... })
class FooComponent implements OnInit {

  $ramdaFoos: Observable<Foo[]>

  /* readonly property added to interface */
  $rambdaFoos: Observable<readonly Foo[]>

  constructor(private store: Store<AppState>){}

  ngOnInit(): void {
    this.rambdaFoos$ = this.store.pipe(select(selectRambdaFoos))
    this.ramdaFoos$ = this.store.pipe(select(selectRamdaFoos))
  }
}

Here is a working stackblitz if you need it.
https://stackblitz.com/edit/rambda-readonly-issue?file=src/app/courses/reducers/courses.reducer.ts
https://stackblitz.com/edit/rambda-readonly-issue?file=src/app/courses/components/course-list/course-list.component.ts

Cheers

[selfrefactor]

The readonly attribute all around is caused by #560

In rambda-scripts repo, there is command yarn immutable which made the change. Not sure what to do, as I see you have a valid point. Skipping immutable only for R.filter won't solve the generic problem.

I tried changing options of eslint immutable plugin, but I didn't saw any change - https://github.com/jonaskello/eslint-plugin-functional/blob/master/docs/rules/prefer-readonly-type.md

I personally don't like readonly as it makes files/index.d.ts hard to read and at one point I removed all readonly, even those that helps. Then because of #560 I had to made the opposite change.

I am open to any suggestions how to solve this pickle.

[synthet1c]

I removed all the immutable return types keeping the immutable input types in my local version of index.d.ts which solved the issue, but it's a big change, and I can't run all the tests to check if anything breaks until I get my local dev environment working.

It would make it easier to work with the library as at the moment I would need to change types in hundreds of places and I'm not going to do that, whereas with readonly removed users who want immutable types can specify it in their own code and the rambda types will respect it in both cases.

I could probably make a mutable higher order function to get around it but it still increases the complexity and code noise.

function mutable<T extends Object>(x: readonly T): T:
function mutable<T extends Array>(x: readonly T[]): T[] {
  if (Array.isArray(x)) {
    return [...x]
  }
  return { ...x };
}

const selectFoos = createSelector(
  selectAllFoos,
  compose(
    mutable,
    filter<Foo>(identity),
  )
)

I don't know enough about typescript to know the impact, I would think it's small if it was only added in November and it seems to be related to tslint only. Maybe @peterreisz can comment.

[selfrefactor]

I agree that your last code is something one wants to avoid. Let's wait for Peter to join in and meanwhile I'll think if it is possible to keep somehow both versions(with and without readonly). Maybe by double release - publish 7.5.2 without readonly and 7.5.2-fp with readonly.

[synthet1c]

Could you have a folder for immutable types using it's own typescript definition, that way there isn't two versions of the library and instead just two different typescript definition files.

import { filter } from 'rambda/strict` // => reference `/strict/index.d.ts` for types
import { map } from 'rambda` // => reference `/index.d.ts` for types

/strict/index.ts

export * from '../rambda;

I can see a reason why someone would want readonly, but rambda is implicitly immutable, making internal functions strictly immutable doesn't really change anything.

[peterreisz]

Making everything readonly is a safeguard for immutability to prevent bugs. I not once saw when the dev wanted to set just a property in an object introducing some unwanted behaviour.
This and typescript compiler options like strict gives you the power to write more reliable code.
I highly recommend to change your mind about it, otherwise you just downgrade typescript to the level of javascript.
The compiler is a great tool, which you should use as assistant and turn the runtime bugs into compilation errors.

We are using readonly in a pretty big project with ngrx, the whole state and actions are readonly too.
If you are using the selector output directly on the template I don't understand what is the problem with it.
By the way, I prefer the following format, so you don't have to deal with types:

export class CourseListComponent implements OnInit {

  beginnerCourses$ = this.store.select(selectBeginnerCourses);
  advancedCourses$ = this.store.select(selectAdvancedCourses);

  constructor(private store: Store) {}

  ngOnInit(): void {
    this.store.dispatch(CourseActions.loadAllCourses());
  }
}

[selfrefactor]

Thank you @peterreisz for joining in. I also haven't faced any issue with readonly and my only reasoning was readability of files/index.d.ts.

I'd suggest indeed something along the lines of import { filter } from 'rambda/foo. The question here what will be the name and content of foo.ts. Should the preference go to with or without readonly? I think that the preference must be in favor of readonly as we create a braking changes otherwise. In this case, we need a better naming than foo to indicate that the types are not so strict.

[synthet1c]

import { whatever } from  'rambda/unsafe'

If it's not a problem, then it's not a problem and this should be closed, I'm only learning angular so if I'm doing the wrong thing the library shouldn't change to suit me.

[selfrefactor]

@synthet1c I agree with the naming. The issue will be closed after the changes are made and new version released. This will happen soon enough, but I cannot give now a hard date.

[peterreisz]

Your welcome @selfrefactor. I prefer readonly.

In case of you want to introduce an alternative variant you could choose:

import { whatever } from  'rambda/non-strict'
import { whatever } from  'rambda/non-readonly'

I think unsafe indicate something bad.

@synthet1c Its not only angular, but typescript as well. Find some good material on the topic, there are some interesting parts of it, for example typeguards. If you prefer videos, Udemy has some great courses. Also I can recommend egghead.io as well.

[selfrefactor]

I agree with Peter, that unsafe is maybe not the best option. I though initially for non-strict but I find it a bit verbose, even if it is perfect descriptor. I'd suggest also loose as alternative to non-strict.

I'll leave Andrew choose the word. Possible options:

• loose
• non-strict

This issue will also require proper change in documentation as users should be informed for the alternative option.

[synthet1c]

This is way too much responsibility for me.

The teenager in me likes "loose" for the lolz, but it sounds a bit unprofessional and doesn't infer the meaning even though it is the opposite of strict, "loose-types" might be better.

"unsafe" was a nod to react building in a little shame for using the bad practice and as a contract between the library and the developer both parties agree to. every time I use dangerouslySetInnerHTML I show respect to the security concerns of the method, but I agree without knowing about it QA might be worried about including a library that is "unsafe". I think I got it from a Brian Lonsdorf video on the IO ADT.

"mutable" isn't really being honest as nothing is mutable inside the library, just the type signatures aren't explicitly immutable.

"legacy" doesn't really convey the meaning.

"non-strict" is the most direct and descriptive even though it's a negative statement. You don't need to consult any documentation to understand what it means.

I tend to favour "non-strict".

[selfrefactor]

So it is agreed to be non-strict. As it is not a trivial task, it might take up to two weeks before a release is made. I'll write again, when there is a progress.

[selfrefactor]

I think that, while we have an agreement, this will complicate already complicated build and test process.

What I will do is that there will be a script to remove readonly types(we already have script to return them back).

I can use this script to make a new NPM release in the form of X.X.X-loose(or X.X.X-beta if that is not possible), which will be the same version of X.X.X but without readonly types.

This process will be manual, so after the next release, I will make another release with aforementioned appendix. The timeframe remains the same.

[peterreisz]

The main problem is if the functions returns with ReadonlyArray<T> the result cannot be directly passed to a function or type which is T[]. But for a readonly parameter you can pass a non-readonly type.

I was thinking about the problem and I might have a midway solution:
All input parameters should be readonly T[]. All output parameters should be T[]. The lib is immutable, it always returns a new array, so it is not a problem if the returned value is mutable in the typescript definition.

I think we can apply this to other methods as well, all the input params should be Readonly<T> the output just T.

This way the projects using the lib enforces the readonly linting rule can stil continue to do so, but who do not want to use it this way is not forced to.

[selfrefactor]

That would work great. I am just unsure how to apply the change. From what I see, ESLint immutable plugin doesn't offer such option, but I might be wrong.

[peterreisz]

Haven't tried, but here there is an option for it:
https://github.com/jonaskello/eslint-plugin-functional/blob/master/docs/rules/prefer-readonly-type.md

allowMutableReturnType: boolean;

[selfrefactor]

I added the option, then removed readonly from the return type of R.append. When I ran yarn immurable from rambda-scripts, the change is reverted.

[peterreisz]

I've checked and it seems it doesn't support linting type definition files. I think the issue is occurs when there is no function body.

Try tslint instead, it's a deprecated project, but seems working for me.

yarn add tslint tslint-immutable

{
    "extends": ["tslint-immutable"],
    "rules": {
        "readonly-keyword": true,
        "readonly-array": [true, { "ignore-return-type": true }]
    }
}

There might be a problem with the object return type, there is no option for ignoring readonly for those.
In case of arrays the cause of compilation problems is the missing methods, but I think it could work with object.

[selfrefactor]

I see that we have found a bug in ESLint plugin. Which is nice, as if it is resolved, then our solution is easy. I'd prefer first fixing it with a command, and if not successful, then I'll give tslint-mutable a chance.

[Retro64]

When migrating a project from ramda to rambda we also stumbled over this issue at work (we used ramda/map with DefinitlyTyped types). So, at least from our point of view, the readonly return values interfere heavily with the ramda interface - you find rarely types for ramda functions returning readonly values (so no longer "may vary", but definitely breaks). The immutability is - as here suggested - ensured by marking the input parameters readonly. But we know ramda used to return readonly values in the past - I did not check when they changed the paradigm.

Nevertheless we found some inconvenient workaround in an old ramda issue:

const foo: Foo[] = [];
let bar: Bar[];
const readonlyBar = map(fooToBar, foo); // returns readonly Bar[], not assignable to bar
bar = [...readonlyBar];

[selfrefactor]

Thank you @Retro64 for your input, but I fail to understand your suggestion. Otherwise, I haven't forgotten the issue and I will move it forward soon.

[Retro64]

I guess the proposal was to make the parameters readonly to ensure immutability - I just pointed out this would also match the current ramda interface/types and would therefore be - at least from my point of view - the preferable way to go.

And I wanted to provide the workaround besides an - following my example - bar = readonlyBar as Bar[];. I actually did not find the source of this workaround by creating a new array by spreading the readonly array anymore, but I thought it might be useful to share it in orderto help others stumbling over this issue, when migrating from ramda to rambda...

[Retro64]

One question regarding this issue:

Would it makes sense to add this in the README under "differences between ramda and rambda"? As far as I remember, the ramda types in Definitly typed were changed to avoid returning readonly in the past (see map as example https://github.com/DefinitelyTyped/DefinitelyTyped/blob/f7ec78508c6797e42f87a4390735bc2c650a1bfd/types/ramda/index.d.ts#L1030)

[selfrefactor]

I am unsure what to list as a difference. From what I see, @types/ramda still returns readonly for R.map. Am I missing something @Retro64 ?

[Retro64]

As far as I remember, they changed the signature in the past not to return readonly anymore - at least for some functions (I simply post map as example)

Current @types/ramda

/**
 * Returns a new list, constructed by applying the supplied function to every element of the supplied list.
 */
export function map<T, U>(fn: (x: T) => U, list: readonly T[]): U[];
export function map<T, U>(fn: (x: T) => U): (list: readonly T[]) => U[];
export function map<T, U>(fn: (x: T[keyof T & keyof U] | ValueOfUnion<T>) => U[keyof T & keyof U], list: T): U;
export function map<T, U>(fn: (x: T[keyof T & keyof U] | ValueOfUnion<T>) => U[keyof T & keyof U]): (list: T) => U;
export function map<T, U>(fn: (x: T) => U, obj: Functor<T>): Functor<U>; // used in functors
export function map<T, U>(fn: (x: T) => U): (obj: Functor<T>) => Functor<U>; // used in functors

Current Types in rambda

/**
 * It returns the result of looping through `iterable` with `fn`.
 * 
 * It works with both array and object.
 */
export function map<T, U>(fn: ObjectIterator<T, U>, iterable: Dictionary<T>): Dictionary<U>;
export function map<T, U>(fn: Iterator<T, U>, iterable: readonly T[]): readonly U[];
export function map<T, U>(fn: Iterator<T, U>): (iterable: readonly T[]) => readonly U[];
export function map<T, U, S>(fn: ObjectIterator<T, U>): (iterable: Dictionary<T>) => Dictionary<U>;
export function map<T>(fn: Iterator<T, T>): (iterable: readonly T[]) => readonly T[];
export function map<T>(fn: Iterator<T, T>, iterable: readonly T[]): readonly T[];

At least I do not see a readonly on the current @types/ramda as return type and as far as I remember, I had some effort in the past when bumping the version (when we still used ramda).

[selfrefactor]

Thanks

I didn't see it, thank you for posting the full example @Retro64

Then I agree that this should be included in the list of differences.

[selfrefactor]

I also saw the issue of readonly and I agree that it forces the user to use readonly and this is not intended. I will add immurable.ts file and users can import rambda/immutable to access typings with readonly(i.e. the current state of Rambda typings). The change is coming in 6.8.0 and I'll write again when the version is ready.

[synthet1c]

Hey Dejan, So happy that I can use this in my projects at work with readonly fixed. I'm trying to replace the rambda/index.d.ts with my own file but can't get the tsconfig right. Could you help me with instructions on how to do it?

This is my tsconfig.json, I'm attempting to use the include and exclude properties to overwrite the base index.d.ts is this the correct way, or how should I do it?

{
  "compileOnSave": false,
  "compilerOptions": {
    "baseUrl": "./",
    "outDir": "./dist/out-tsc",
    "sourceMap": true,
    "declaration": false,
    "moduleResolution": "node",
    "emitDecoratorMetadata": true,
    "experimentalDecorators": true,
    "target": "es5",
    "typeRoots": [
      "node_modules/@types",
      "src/types/"
    ],
    "lib": [
      "es2017",
      "dom"
    ]
  },
  "files": [
    "src/main.ts"
  ],
  "include": [
    "src/types/rambda.d.ts"
  ],
  "exclude": [
    "node_modules/rambda/index.d.ts"
  ]
}

Also do you know when you will deploy 6.8 to npm?

[selfrefactor]

Hi @synthet1c

I am a bit late with the release of 6.8.0 as I wanted to release it much earlier. It won't take longer than 2 weeks.

As for your question of replacing Rambda types - I will try do that on a demo project and I'll get back to you.

[selfrefactor]

6.8.1 was published with removed readonly types.

readonly version is still available by importing rambda/immutable instead of rambda.

Closing the issue, but please feel free to comment further.

[Retro64]

Instant feedback: Works like a charm. Thank you!

[synthet1c]

Working well for me! I had an issue with using an old version of typescript but could easily polyfill the missing definitions. Thanks for your hard work over the weekend to bring it to us ahead of schedule!

[ivan-kleshnin]

My 5 cents. readonly is one of the worst features ever added to TypeScript. I can't remember a single case where it prevented a bug in my code (the habit of not using mutable operations pays off) but I surely remember countless times it bothered me and slowed me down. It has a divisive effect on code bases: you have to either remove it alltogether or add it everywhere (which is never convenient).

Hopefully they'll change it to a "readonly by default" mode (there's an issue for that) and everyone will be happy.