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

remove readonly from query result arrays #266

Closed
wants to merge 1 commit into from

Conversation

dwickern
Copy link
Contributor

This PR removes the readonly flag from arrays returned by query methods (query, any, anyFirst, many, manyFirst).

To summarize #218, this change improves interop between slonik and the broader typescript ecosystem.

This won't break anything for users who want readonly still, since T[] is assignable to readonly T[]:

// still works
const users: readonly User[] = await connection.any(sql`select * from users`)

Fixes #218
Closes #263

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.185% when pulling 830bb50 on dwickern:readonly-array into 52255ae on gajus:master.

@kevinkoste
Copy link

Adding my support for merging this PR!

@gajus gajus added the wontfix label Jul 21, 2021
@MatthewHerbst
Copy link

@gajus can you please expand on your wontfix label here (and why the PR is still open)? Is there another solution to this PR? Thank you

@gajus
Copy link
Owner

gajus commented Aug 11, 2021

This was already discussed and my opinion remains that these resources are support to be read-only. If someone want to turn them into mutable arrays, then there are simple workarounds (e.g. .slice()).

I understand that it is painful if you are not already using readonly in your application, but that seems more like an oversight in your application than anything else.

I kept this PR open because I wanted to investigate if there is a way to make this configurable, i.e. Could pool be initiated with mutableArrays option. Not high on priorities list, though.

@gajus gajus closed this Aug 11, 2021
@MatthewHerbst
Copy link

then there are simple workarounds

None (to my knowledge) that don't involve runtime changes unfortunately, and added runtime code to make types happy is not an acceptable tradeoff.

For my org we pipe the results of the Slonik query directly into a 3rd party library so we have no control over the types there unfortunately.

Thanks for the response. If only more people used readonly everywhere, there would be no problem 😃

@NicholasDawson
Copy link

NicholasDawson commented Jul 17, 2023

This was already discussed and my opinion remains that these resources are support to be read-only.

@gajus From everything I read in this repo no one supported keeping readonly except yourself. When you said you can't think of valid use cases, people gave you some. Why not listen to the community?

Type checking is valid for knowing and validating the output data structure, but restricting the output of a function to be readonly is an unnecessary restriction.

People are using this library and it is returning data as a result. That data is unrelated to slonik, why make it readonly and restrict it?

I could make the same argument about simple workarounds, if you want your response to be readonly then make it readonly yourself explicitly. Slice happens at runtime and will have performance impacts to get rid of a compile time check...this makes no sense.

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

Successfully merging this pull request may close these issues.

Question: Why are query results marked as readonly?
6 participants