Skip to content

Refactor store to remove readonly - #790

Closed
otonashixav wants to merge 3 commits into
solidjs:mainfrom
otonashixav:refactor-store-remove-readonly
Closed

Refactor store to remove readonly#790
otonashixav wants to merge 3 commits into
solidjs:mainfrom
otonashixav:refactor-store-remove-readonly

Conversation

@otonashixav

@otonashixav otonashixav commented Jan 10, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

After changing Store to be readonly, @high1 pointed out on discord that non-readonly types might be more desirable e.g. in this scenario:

function getFirst(list: string[]): string {
  return list[0];
}

const [store, setStore] = createStore({ list: ["hello"] });

getFirst(store.list) // <-- readonly string[] not assignable to string[]
setStore("list", (list) => [getFirst(list)]) // <-- readonly string[] not assignable to string[]

It's fairly common to encounter immutable functions that do not type their parameters as readonly outside of native functions/classes like Object.fromEntries or Map. This PR makes it so none of these are readonly. However, I'm not sure if setStore should be kept as readonly.

In my opinion:

  • In setStore, there are fewer cases where a user might pass list in setStore to a function typed with string[] if it is immutable, and more cases where users to try to directly modify list within setStore if it is mutable.
  • In store, a user might attempt to call methods that do not exist on readonly arrays such as store.list.push("uh oh") if it is mutable.

Hence a discussion might be needed on whether it should be:

  1. readonly store and setStore: technically "correct" types, but might require casting
  2. mutable store and readonly setStore: less likely to get in most users' way, but allows store to be misused e.g. store.list = [], store.list.push("uh oh"). Inconsistent.
  3. mutable store and setStore: definitely will not get in users' way, but additionally allows setStore to be misused e.g. setStore(store => (store.list = [], store)).

@coveralls

coveralls commented Jan 10, 2022 •
edited
Loading

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 1692494664

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 13 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.02%) to 89.381%
Files with Coverage Reduction New Missed Lines %
packages/solid/src/render/Suspense.ts 1 80.49%
packages/solid/web/src/index.ts 2 81.69%
packages/solid/src/render/component.ts 10 64.96%
Totals Coverage Status
Change from base Build 1676947832: -0.02%
Covered Lines: 1193
Relevant Lines: 1275
💛 - Coveralls

@otonashixav
otonashixav marked this pull request as ready for review January 12, 2022 07:09
@otonashixav otonashixav mentioned this pull request Feb 1, 2022
Closed
@trusktr

trusktr commented Feb 6, 2022

Copy link
Copy Markdown
Contributor

I have a feeling that keeping it correct (all readonly) is the way to go.

It is similar in practice to writing let foo for a variable that never changes, when we should be using const instead.

In a similar vein, if a function is only reading, it should be defined with readonly for it to be correct (how many of us forget to do that :), let's add lint rules).

playground example that shows the "right" and "wrong" ways to write code.

It's fairly common to encounter immutable functions that do not type their parameters as readonly outside of native functions/classes like Object.fromEntries or Map.

I think it is an artifact from a time before readonly was in place.

I think it is better to keep people accidentally trying to modify state and things not working, or passing state to some APIs and expecting it to perform state management and it not working, etc.

@high1

high1 commented Feb 6, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

The issue here is the usage of the store - this forces the user to unbox every value that he reads from the store, and that's visible from the example, right? Although the value is only read and not mutated in any way, you are forced to unbox it from readonly for reading.

TypeScipt has Readonly which does not force the user to unbox, but gives a compiler error on mutation. So, I strongly feel that the current typing is not good enough.

@trusktr

trusktr commented Feb 6, 2022

Copy link
Copy Markdown
Contributor

If you can post playground examples, that'd be helpful.

@non25

non25 commented Feb 6, 2022 •
edited
Loading

Copy link
Copy Markdown

I guess it's better to use _.cloneDeep, spread or override some library types globally in the project.

https://tsplay.dev/WkMEPN this doesn't shout at the dev sadly. 😞

@non25

non25 commented Feb 7, 2022 •
edited
Loading

Copy link
Copy Markdown

https://tsplay.dev/WvVkBW this is actually great and will prevent shooting ourselves in the foot in case we try to do pure functions without immer.
I've seen .sort() and deep object spreads with nested properties mutation in "pure" functions.
DeepReadonly would shout on them.

I vote on keeping it as it is.

@atk

atk commented Feb 7, 2022

Copy link
Copy Markdown
Contributor

I feel this is more breaking than making the types. Can we instead provide an Unwrap<Store> helper type for those rare cases when you really need to abuse stores in such a way?

@otonashixav
otonashixav marked this pull request as draft February 12, 2022 11:26
@otonashixav otonashixav mentioned this pull request Feb 12, 2022
Merged
@otonashixav
otonashixav deleted the refactor-store-remove-readonly branch April 28, 2022 05:05
@coveralls

coveralls commented Nov 16, 2024

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 1692494664

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 13 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.02%) to 89.381%
Files with Coverage Reduction New Missed Lines %
packages/solid/src/render/Suspense.ts 1 80.49%
packages/solid/web/src/index.ts 2 81.69%
packages/solid/src/render/component.ts 10 64.96%
Totals Coverage Status
Change from base Build 1676947832: -0.02%
Covered Lines: 1193
Relevant Lines: 1275
💛 - Coveralls

1 similar comment
@coveralls

coveralls commented Nov 16, 2024

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 1692494664

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 13 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.02%) to 89.381%
Files with Coverage Reduction New Missed Lines %
packages/solid/src/render/Suspense.ts 1 80.49%
packages/solid/web/src/index.ts 2 81.69%
packages/solid/src/render/component.ts 10 64.96%
Totals Coverage Status
Change from base Build 1676947832: -0.02%
Covered Lines: 1193
Relevant Lines: 1275
💛 - Coveralls

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@otonashixav @coveralls @trusktr @high1 @non25 @atk