Question: Why are query results marked as readonly?

[neilpoulin]

(sorry this isn't a bug, but can't figure out how to remove the label)

I just upgraded to version 23.0.1. I use typescript and was excited to see that the library had been re-written in typescript.
Overall the upgrade seemed to go smoothly. However, on my .many<MyObjectType>() calls, I had type errors saying that the result was marked as readonly, and could not be cast to mutable type MyObjectType[].

I'm writing just to ask why this is the case. I'm able to get around this and return my mutable array type by spreading the results into a new array.

Here's a more complete example:

// Original code, with type error
async function getResponses(): Promise<UserAssessmentResponse[]> {
    const responses: UserAssessmentResponse[] = await this.db.pool.many<UserAssessmentResponse>(sql`
        SELECT *
        FROM user_assessment_response
        WHERE user_assessment_id = ${userAssessmentId};
        `);

    /** TYPE ERROR: 
    The type 'readonly UserAssessmentResponse[]' is 'readonly' and cannot be assigned to the mutable type 
    'UserAssessmentResponse[]
    */
    return resposnes
}

// "fixed" code
async function getResponses(): Promise<UserAssessmentResponse[]> {
    const responses: readonly UserAssessmentResponse[] = await this.db.pool.many<UserAssessmentResponse>(sql`
        SELECT *
        FROM user_assessment_response
        WHERE user_assessment_id = ${userAssessmentId};
        `);

    return [...responses]
}

[mmkal]

I did the switch, and the reason the readonly keywords are there is basically because the flow codebase marked them readonly too. I'd also be interested why - in general I prefer to use a linter to defend against mutations than the type system.

For your example - do you have to have all the type defs? The following should work:

async function getResponses() {
    return this.db.pool.many<UserAssessmentResponse>(sql`
        SELECT *
        FROM user_assessment_response
        WHERE user_assessment_id = ${userAssessmentId};
    `);
}

Alternatively, if your project requires explicit typedefs, you could just add readonly to the function return type too.

[neilpoulin]

Thanks, @mmkal

I could do as you suggested, but in other places that I use the returned responses array, say another function that does something with it, I'd need to update the type definitions to mark it as readonly, too, which I just don't really want to do unless there was some clear benefit in these values being set as readonly.

[gajus]

I cannot think of a single valid use case where one would want / should mutate object or an array. Everything in your codebase that is either a parameter or a return value should be read only. Otherwise, you are effectively creating side-effects and hard to debug issue.

However, I am still only beginning to adopt TS and perhaps readonly is somehow treated differently that how I expect. Do you have a code example to share that is breaking with these changes?

I am closing this as there is no action for now, but please continue the conversation.

[mmkal]

@gajus I agree, it's very rare to have a valid use case for mutating arrays anywhere. The only reason I'd advocate for dropping the readonly keyword is that it's not really slonik's job to force library users not to shoot themselves in the foot. Or to jump through hoops like forcing them to add readonly to their own typedefs, or the [...results] from the example above. And if users really want to mutate arrays... well it's their gun, and their foot.

There are interop problems too. Not all libraries use readonly. A example - inline-loops.macro accepted a regular array in the type definitions, meaning readonly arrays couldn't be passed to the map function. Before I removed it from this codebase, it had to be patched. Patching is a pretty extreme solution and a lot of developers will reach for as any instead to avoid the headache. Similarly, a few teams I've been on use https://npmjs.com/package/eslint-plugin-functional to prevent mutations without littering typedefs with readonly everywhere. There's less overhead and it's more interoperable.

[neilpoulin]

This is a great discussion, thank you both.

While I'm no TypeScript expert, my understanding is that the readonly keyword is meant to modify access to class properties, and isn't really meant for function parameters.

I also tend to agree with @mmkal about the library not dictating how our return values can be used, especially since at the end of the day it's just a javascript array with no special protections (other than the type system).

Another alternative that would make more sense to me from a TypesScritpt standpoint is to use something like the built-in ReadonlyArray. It would be more clear to me when looking at return types and function signatures if this was used vs the readonly keyword.

Here's another resource I found useful:
https://basarat.gitbook.io/typescript/type-system/readonly#various-use-cases

And here's a counter argument for using the readonly keyword as it is currently implemented
https://mariusschulz.com/blog/read-only-array-and-tuple-types-in-typescript

In summary, I just found the readonly modifier to be unexpected, particularly since I had existing code using the previous major version (i.e. before the documented breaking change). If I had come into the library fresh, not having upgraded versions, I don't know if I would have thought twice about it.

Thanks for a great library and a thoughtful discussion!

[seizir]

@gajus

Everything in your codebase that is either a parameter or a return value should be read only.

There is a large difference between making parameters read only and making return values read only.

Read only parameters tell the user that the function will not mutate the parameters but will accept both mutable and immutable values.

Read only return values prevent the user from mutating the value and passing it to other functions (some maybe in third party libraries) that don't have their parameters marked read only.

Updating to version 23 in our project would require changing types across the entire code path the returned arrays are used in or resorting to tricks already mentioned in this issue. Even then we would still probably run into issues with other libraries that don't have everything marked read only.

I believe that mutating results is a valid use case and not something Slonik should prevent the user from doing.

The only use case for a read only return value I can think of is if the returned object was a direct reference to some internal state that can be read but not modified. This is not the case here.

I have a C++ background where const is used as much as possible but using it for return values is rarely seen.

If someone does wish to prevent their query results from being mutated they can do so by assigning to a read only variable or changing the return type of the function in their own code. I imagine most people write code similar to the example in the OP. This would avoid the issues caused while providing all the same benefits.

[mutru]

@gajus I agree with encouraging the functional style, but I need to second the previous comment: making return values readonly makes it really difficult to work in the current TypeScript ecosystem, where readonly is not as widely used.

Our team is currently discussing writing a wrapper around Slonik to change these types, because updating everything to use readonly would be too much work.

[mikeporterdev]

@gajus This makes slonik near unusable for us as we would need to propagate the keyword readonly throughout our system everywhere these types are dealt with as our project specifies many types explicitly. This is unwieldy and in my opinion is an example of slonik being too opinionated about how users should deal with the outputs from your library.