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 drop of readonly modifiers #65

Open
Nodonisko opened this issue Jan 10, 2023 · 8 comments
Open

Consider drop of readonly modifiers #65

Nodonisko opened this issue Jan 10, 2023 · 8 comments

Comments

@Nodonisko
Copy link
Contributor

Nodonisko commented Jan 10, 2023

readonly is constant source of pain, because everytime where code from ts-belt come to contact with normal JS code it throws type error. It happens us a lot because we are using it in legacy codebase a even if you do thing like simple A.map you need always wrap result in F.toMutable.

If I decide to refactor our code to work with it, I need to place readonly literally everywhere. That will break TS in our tests because of our fixtures which are mutable. Same problem is that everywhere where code come to touch with 3rd party libraries I will throw some errors about readonly again.

And last and probably strongest argument is this nice thread on Twitter with some great examples why it's even not that safe >>> https://twitter.com/MichaelArnaldi/status/1601535981949456385

I know this is will produce big change (but shouldn't be breaking) and final resolution is on you @mobily but please consider dropping it for v4.

@mobily
Copy link
Owner

mobily commented Jan 10, 2023

hello @Nodonisko 👋 thanks for the suggestion, and I will definitely consider it, in the meantime, have you tried this https://mobily.github.io/ts-belt/docs/getting-started/config#immutable-vs-mutable?

@Nodonisko
Copy link
Contributor Author

@mobily Cool I didn't know about that config! I will try it :)

@mobily
Copy link
Owner

mobily commented Jan 10, 2023

and the moment it supports arrays only, but I think adding support for objects doesn't require much effort

@mobily
Copy link
Owner

mobily commented Jan 31, 2023

@Nodonisko does that work as intended on your end?

@Nodonisko
Copy link
Contributor Author

@mobily It's better, but as you mentioned without object support there are still places where it throws a lot.

@Nodonisko
Copy link
Contributor Author

Nodonisko commented Feb 3, 2023

I think I can help you with this one, but I have troubles to understand how to contribute. For example for Array, there are two files:

/Array/Array.ts
/Array/index.ts

In index.ts there seems to be type definitions for all functions from Array.res but there is also file Array.ts where is only small subset of type definitions for these functions. So which files I can change?

@lieberkind
Copy link

lieberkind commented Mar 7, 2023

and the moment it supports arrays only, but I think adding support for objects doesn't require much effort

@mobily Adding a mutability option for objects would be a great help! At my company we're also trying to introduce ts-belt for into a large codebase and are getting hit by having to run F.toMutable a lot.

Thanks for the great work!

@adrian-gierakowski
Copy link

adrian-gierakowski commented Jan 9, 2024

to add my two cents: it's best if function arguments are immutable, but return values not. This allows for seamless integration with libraries which are imprecise in their types and use mutable types in function arguments, event though they do not mutate them

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

No branches or pull requests

4 participants